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

Fix review dog #50

Merged
merged 4 commits into from
Oct 19, 2024
Merged

Fix review dog #50

merged 4 commits into from
Oct 19, 2024

Conversation

wusatosi
Copy link
Member

@wusatosi wusatosi commented Oct 18, 2024

closes #49

Reviewdog introduced in #46 is lacking permission to create annotations, see #49 for error logs on #48, commit: 6ec3d4f .

Also

  1. fixed use of deprecated fail-on-error, causing warning at: https://github.com/beman-project/exemplar/actions/runs/11411671176/job/31756356511?pr=48#step:7:234
  2. Removed unnecessary caching behavior, see: feat: add automatic linting suggestion github workflow #46 (review)
  3. Only trigger review dog on pull request (currently commits will also trigger review dog).

See example at: wusatosi#11

CC: @linusboehm

@linusboehm
Copy link
Collaborator

Thanks for the change! I'm not quite sure how the permissions work, but I think I recall that the permissions have to be set in the repository settings to allow workflows to comment on a commit.

@camio camio merged commit 9aa88cd into bemanproject:main Oct 19, 2024
15 checks passed
@wusatosi
Copy link
Member Author

Thanks for the change! I'm not quite sure how the permissions work, but I think I recall that the permissions have to be set in the repository settings to allow workflows to comment on a commit.

Its interesting that only pull-requests: write is not sufficient to leave reviews...
I don't know what's going on as well.

@camio
Copy link
Contributor

camio commented Oct 19, 2024

@wusatosi I attempted to make you a member of the org. Let me know if that doesn't give you authorization to change permissions as needed.

@linusboehm
Copy link
Collaborator

@camio i don't think that's sufficient. I think someone who's able to change the repository settings needs to change a setting (I can look it up. I think I've done it before). However, this might be a problem for the auto-commenting in general, because I'm not sure changing the setting can be automated. So it might need some manual setup/setting change for every repository that wants to use that feature.

@@ -39,8 +35,9 @@ jobs:
continue-on-error: true

- name: suggester / pre-commit
if: ${{ github.event_name == 'pull_request' }}
Copy link
Member Author

Choose a reason for hiding this comment

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

AH I just realized this breaks linting for comments.

Pre-commit will fail but it has continue on failure set to true for review dog to run, and in commits review dog doesn't run so the check will show up as passed.

I don't think there's anyway to fix this within this job, I have to make a separate job that triggers on commit.

Damn it!

@wusatosi
Copy link
Member Author

[Simply giving me org member doesn't help]

Let me try to open a pull request to check.

@linusboehm
Copy link
Collaborator

Thanks for the change! I'm not quite sure how the permissions work, but I think I recall that the permissions have to be set in the repository settings to allow workflows to comment on a commit.

Its interesting that only pull-requests: write is not sufficient to leave reviews...

I don't know what's going on as well.

I tested it with pull-request: write only here: linusboehm#2
So I don't think the other two permissions that were added are necessary

@linusboehm
Copy link
Collaborator

linusboehm commented Oct 19, 2024

this might be the reason: https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#how-the-permissions-are-calculated-for-a-workflow-job

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.

@camio so seems like a setting change is required (member of the org is not sufficient, as I can't access the settings)

@wusatosi
Copy link
Member Author

wusatosi commented Oct 19, 2024

this might be the reason: https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#how-the-permissions-are-calculated-for-a-workflow-job

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.

@camio so seems like a setting change is required (member of the org is not sufficient, as I can't access the settings)

I opened a testing pr to check, See #53 , I think linus you are right here.

@wusatosi
Copy link
Member Author

I reopened #49 , let's forward the discussion there.

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.

Review dog failing in CI due to insufficient permission
3 participants