Usage documentation wrong

[wollew]

The README currently describes the "basic usage" as a 2 step process, where CI does not need/get permissions because pulled code is potentially untrusted. But the example does exactly that:

# .github/workflows/ci.yml
    ...
    permissions:
      # Gives the action the necessary permissions for publishing new
      # comments in pull requests.
      pull-requests: write
      # Gives the action the necessary permissions for pushing data to the
      # python-coverage-comment-action branch, and for editing existing
      # comments (to avoid publishing multiple comments in the same PR)
      contents: write

My understanding is that these permissions should only go in .github/workflows/coverage.yml. Is there a misunderstanding on my side or should the docs be adjusted?

[ewjoachim]

I wasn't able to find the page again, but there is somewhere a page of the GitHub docs that says that for on: pull_request (not pull_request_target), when executed from a fork from someone not admin, the maximum level they will get is no secrets and readonly, whatever the permissions: say. I'd love to be able to pinpoint the page, but I wasn't able to find it.

[ewjoachim]

At least, there:

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#workflows-in-forked-repositories

But I'm sure I saw a while back a table with this said more explicitly. At least, there one source.

Probably that the easiest way to prove it would be to write a quick repo to test.

[wollew]

I think I found it at least somewhat explicit here:

Finally, if the workflow was triggered by a pull request from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read only.

So the provided example in the README doesn't hurt in giving more permissions than needed (github will just downgrade to contents: read), but it is also superfluous to add because it doesn't really do anything anyway.

[ewjoachim]

but it is also superfluous to add because it doesn't really do anything anyway

No, it will have a effect for users with read-write access.
If your workflows are configured to run with read-only permissions by default (which is a good idea), then you need to have these permissions on if you want to write.

When the "normal" part of the workflow runs, if python-coverage-comment-action detects that it can publish the comment directly without needing the 2nd part of the action, it does so immediately and we skip the 2nd part.

[ewjoachim]

(If you think this is badly explained in the readme, you're probably right, feel free to improve the wording)

[wollew]

Let's see if I understand this correctly: for PRs from someone with write acces (or maybe I should say: not a PR from a forked repo), this can help. And for PRs from a forked repo, it doesn't have any effect and therefore doesn't hurt either. If this how you see it as well, then this issue can be closed, I was wrong and not the docs :-)

[ewjoachim]

You summed it up well :)

I'll let you close if you don't think its worth spelling this out explicitly in the docs. I think it might be worth a paragraph :)