Conversation
Be more explicit about what is tested to avoid confusion when reading the logs.
didrocks
left a comment
didrocks
left a comment
There was a problem hiding this comment.
A couple of nitpicks! Otherwise, good :) Thanks!
| ref: ${{ github.event_name == 'pull_request' && github.head_ref || github.ref }} | ||
| token: ${{ inputs.token }} | ||
| - name: Check push permissions | ||
| - name: Check token has no contents:write permission |
There was a problem hiding this comment.
This is quite specific and explain what the step does rather than the intent, isn't it?
There was a problem hiding this comment.
My issue with the old name is that it doesn't make it clear what the check expects the permissions to be. When I read "Check push permissions" in the logs, I assumed that it would check that the token has push permissions. I had to look at the implementation to understand that it checks the opposite, that the token has no push permissions.
There was a problem hiding this comment.
We could change it to "Check no push permissions". On the other hand, contents:write gives more permissions than just pushing, so that description wouldn't be entirely accurate either.
There was a problem hiding this comment.
here is the entire list of APIs which the contents permission gives access to: https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-contents
| # Push just works because we checkout with `persist-credentials: true` (the default | ||
| # behaviour). | ||
| if git push --set-upstream origin "${NEW_BRANCH}"; then | ||
| echo "::endgroup::" # Close the group first so that the error message is visible. |
There was a problem hiding this comment.
You can maybe git push in its own line before the if, save the results, then endgroup and proceed with the ifThat way, you won’t need the else, or am I missing anything?
There was a problem hiding this comment.
I also considered that but I found it more convoluted than the else. I don't have a strong opinion though, so I'll change the implementation.
| # Check token has no pull-requests:write permission | ||
| echo "::group::Check token has no contents:write permission" | ||
| if gh pr comment -b "You should restrict the github token permissions of the '${{ github.job }}' job (ideally to none - 'permissions: {}')" ${{ github.event.pull_request.number }}; then | ||
| echo "::endgroup::" # Close the group first so that the error message is visible. |
There was a problem hiding this comment.
See previous remark here too.
adombeck
force-pushed
the
improve-gh-perms-logs
branch
from
dbdbfde to
db9c9b1
Compare
2 participants
Improve step names and group log lines.