Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set permissions: contents: write #347

Merged
merged 4 commits into from
May 26, 2022

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented May 16, 2022

Setting permissions: contents: write is required in new repositories for the GITHUB_TOKEN to work. See also JuliaDocs/Documenter.jl#1819 and julia-actions/julia-docdeploy#21

Without it, people might get an error like

fatal: unable to access 'https://github.com/rikhuijzer/MyProject.jl.git/': The requested URL returned error: 403

Please double-check whether I haven't missed a place in PkgTemplates.jl where this setting should be set too

@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #347 (bfc50ba) into master (bb3b54d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #347   +/-   ##
=======================================
  Coverage   94.76%   94.76%           
=======================================
  Files          20       20           
  Lines         630      630           
=======================================
  Hits          597      597           
  Misses         33       33           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb3b54d...bfc50ba. Read the comment docs.

@nickrobinson251
Copy link
Collaborator

nickrobinson251 commented May 25, 2022

Thanks for the contribution, @rikhuijzer

Looks like a few more files in test/fixtures/ need the same update for reference tests to pass.

More generally, are there more restricted permissions we could set then write-all that would enable what we want?

PkgTemplates is fairly widely used so whatever permissions we set here can end up being set across many packages, so i'd be keen to set the minimal required :)

@DilumAluthge
Copy link
Member

DilumAluthge commented May 25, 2022

More generally, are there more restricted permissions we could set then write-all that would enable what we want?

Yeah, I strongly recommend only giving the minimum necessary set of permissions.

@rikhuijzer rikhuijzer changed the title Set permissions: write-all Set permissions: contents: write May 26, 2022
@rikhuijzer
Copy link
Contributor Author

rikhuijzer commented May 26, 2022

More generally, are there more restricted permissions we could set then write-all that would enable what we want?

Yes. Well spotted. Would you agree that

    permissions:
      contents: write

is good? This is now used at julia-actions/julia-docdeploy#21. I've tested it at https://github.com/rikhuijzer/DiscountedCashFlows.jl/commits/main commits 4f85500df75d6efc78e999ed78e201a17f74216d and d19106abd4de732fdecc8d64aa1e176058dc855c.

@nickrobinson251
Copy link
Collaborator

Yeah, permissions: contents: write sounds good to me!

I think you'll also need to rebase this on master, as we just updated the reference tests to run on v1.7.2 (#349).
If you then run tests locally with v1.7.2 it should offer the option to update any reference test files that need updating to match this change.

Finally, if you bump the Project.toml version to the next non-breaking version, i'll make a release with this change as soon as it's merged

Thanks again!

Comment on lines +11 to +12
permissions:
contents: write
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this is enough for the register action. The GitHub Actions scope elements are very poorly documented (e.g., https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/)

@rikhuijzer
Copy link
Contributor Author

I think you'll also need to rebase this on master, as we just updated the reference tests to run on v1.7.2 (#349).
If you then run tests locally with v1.7.2 it should offer the option to update any reference test files that need updating to match this change.

Finally, if you bump the Project.toml version to the next non-breaking version, i'll make a release with this change as soon as it's merged

Great. Thanks for the instructions.

I won't have time to continue working on this. If you want to get it merged quickly, then feel free to make some further adjustements

@nickrobinson251
Copy link
Collaborator

thanks for your work, @rikhuijzer

hopefully i can get this over the line from here. i must admit Github Actions is not something i'm too familiar with. I wonder if @DilumAluthge might be able to give this PR a review?

Copy link
Collaborator

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants