fgrosse / go-coverage-report

A CLI tool and GitHub Action to post Go code coverage reports as comment to your pull requests.
BSD 3-Clause "New" or "Revised" License
53 stars 8 forks source link

permissions documentation #32

Closed buzzdan closed 1 month ago

buzzdan commented 2 months ago

thanks for the action! can you add more documentation on the read me about the required permissions on the workflow file?

currently im using:

permissions:
  contents: read
  pull-requests: write

and its still not enough.

at the beginning i got: (before i added those permissions):

Using GitHub's REST API to get changed files
  Getting changed files from GitHub API...
  Error: Resource not accessible by integration - https://docs.github.com/rest/pulls/pulls#list-pull-requests-files

and now i get:

download code coverage results from current run
  + gh run download 9902573423 --name=my-coverage --dir=/tmp/gh-run-download-9902573423
  error fetching artifacts: HTTP 403: Resource not accessible by integration (https://api.github.com/repos/weka/wekapp/actions/runs/9902573423/artifacts?per_page=100)

i;ve added a step before for downloading the artifacts and make sure it has the right permissions:

      - uses: actions/checkout@v4
      - uses: actions/download-artifact@v4
        with:
          name: my-coverage

and it worked:

Preparing to download the following artifacts:
- my-coverage ID: 1693790157, Size: 8545)
...
...
Artifact download completed successfully.
Total of 1 artifact(s) downloaded
Download artifact has finished successfully

so im not sure what is the problem

fgrosse commented 2 months ago

Hi Dan, thanks for raising the point. You are right that the documentation should be updated - currently it is lacking any information on permissions apparently. I can update it but first I would like to understand your case better.

What worked well for me is to add the required permissions directly in the code_coverage job, e.g. like in this example:

https://github.com/fgrosse/prioqueue/blob/254f33ae95114df1d0f08487442e7cfd8c51a39a/.github/workflows/ci.yml#L35-L45

  code_coverage:
    name: "Code coverage report"
    if: github.event_name == 'pull_request'
    runs-on: ubuntu-latest
    needs: unit_tests
    permissions:
      pull-requests: write # write permission needed to comment on PR
    steps:
      - uses: fgrosse/go-coverage-report@v1.0.0
        with:
          sha256sum: "fd199b8feed537124d09b9a02cf92dd16b8854b39af12a318f4119067ece951e"

Within the workflow of the action itself, the same works but there I am also checking out the repo.

Overall, it should not be necessary to use actions/checkout@v4 or actions/download-artifact@v4 separately in order to execute the code coverage comparison.

It is a bit hard to help with your case without seeing the full workflow file. Do you mind sharing a version which was not working?

buzzdan commented 2 months ago

@fgrosse thanks for the reply! i succeeded making it work with those permissions:

permissions:
  contents: read
  actions: read
  pull-requests: write
  issues: write

using actions/checkout@v4 or actions/download-artifact@v4 where just a debugging steps to realize what went wrong i was trying to make sense from the error messages and guess the proper permissions.

so now that it works i can tell that what would help me is: 1) wrap the error messages with your own messages - maybe add recommendation on which permission is missing?

for example you can catch:

Error: Resource not accessible by integration - https://docs.github.com/rest/pulls/pulls#list-pull-requests-files

and throw your own error saying something like missing pull-request read/write permissions

2) the original proposal of adding it to the docs

thanks a lot 🙏

fgrosse commented 1 month ago

Hey Dan, can you check if you actually need issues: write and actions: read ? It seems to work for me with using contents: read and pull-requests: write only.

Wrapping the error messages as you suggested sounds like a good idea. In practice it would mean adding error handling in the bash script that invokes the gh binary to make the API calls. That's a little more involved than what I have time to implement right now so I will only update the documentation and add the two known needed permissions.

Thanks again for raising the issue :)

buzzdan commented 1 week ago

thanks for the update currently i moved to another project so i think documentation update is a great start, if something else rises up, i'll update here