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

Allow updating existing checks #81

Merged
merged 1 commit into from
Dec 26, 2022

Conversation

nineinchnick
Copy link
Contributor

Allow updating existing checks instead of always creating a new one.

I had to update Github dependencies to access github.context.job, which was added a year ago or so.

Without this, there's always a new check run created, with only annotations (example run):
image

With this change, annotations are added to existing jobs, without cluttering the list of jobs (example run):
image

@ghaiszaher
Copy link
Member

@nineinchnick thank you for your contribution. Could you resolve the conflicts so we can take a look?

@nineinchnick nineinchnick requested a review from a team as a code owner December 23, 2022 15:08
@nineinchnick
Copy link
Contributor Author

@ghaiszaher all done!

@ghaiszaher ghaiszaher closed this Dec 23, 2022
@ghaiszaher ghaiszaher reopened this Dec 23, 2022
@ghaiszaher
Copy link
Member

@nineinchnick thank you. Could you please update from master one more time? I fixed the workflow permissions and I need to check if it will run properly now.

Same for the other PR #82

@nineinchnick
Copy link
Contributor Author

Done!

@ghaiszaher
Copy link
Member

Still not working, I can't figure out the correct permissions needed, it could also be related to organization settings. We'll have a look at it later. If you have any ideas let me know.

@nineinchnick
Copy link
Contributor Author

Still not working, I can't figure out the correct permissions needed, it could also be related to organization settings. We'll have a look at it later. If you have any ideas let me know.

Workflows running on forks can only have read permissions for the token: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

This is because they can potentially run untrusted code, without requiring maintainer approval. We're using this action in the Trino project and we had set up a separate workflow to annotate PRs based on test reports saved as artifacts: https://github.com/trinodb/trino/blob/master/.github/workflows/annotate.yml

Notice this is triggered on workflow runs, not PRs, so it executes with a more privileged token.

This probably should be documented here, since it affects anyone that would try to use this action in a public repository.

@nineinchnick
Copy link
Contributor Author

I just found an issue for this: #50

@ghaiszaher
Copy link
Member

Still not working, I can't figure out the correct permissions needed, it could also be related to organization settings. We'll have a look at it later. If you have any ideas let me know.

Workflows running on forks can only have read permissions for the token: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

This is because they can potentially run untrusted code, without requiring maintainer approval. We're using this action in the Trino project and we had set up a separate workflow to annotate PRs based on test reports saved as artifacts: https://github.com/trinodb/trino/blob/master/.github/workflows/annotate.yml

Notice this is triggered on workflow runs, not PRs, so it executes with a more privileged token.

This probably should be documented here, since it affects anyone that would try to use this action in a public repository.

@nineinchnick thanks for the insights, let's track this in #50.

@ghaiszaher ghaiszaher merged commit 25872e3 into ScaCap:master Dec 26, 2022
@nineinchnick nineinchnick deleted the jwas/update-existing-check branch January 3, 2023 15:37
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.

4 participants