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

[GitHub] Set top level token permission #87326

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Conversation

marbre
Copy link
Member

@marbre marbre commented Apr 2, 2024

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 2, 2024

@llvm/pr-subscribers-github-workflow

Author: Marius Brehler (marbre)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/87326.diff

1 Files Affected:

  • (modified) .github/workflows/pr-code-format.yml (+4)
diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index 10b18f245d8965..983838858ba43e 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -1,4 +1,8 @@
 name: "Check code formatting"
+
+permissions:
+  contents: read
+
 on:
   pull_request:
     branches:

@marbre
Copy link
Member Author

marbre commented Apr 2, 2024

Without explicitly setting the token permission, the default (read/write for all scopes) kicks in, if not changed for the entire repo, see https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token.

@tru
Copy link
Collaborator

tru commented Apr 2, 2024

This seems reasonable of course, but as always with these kinds of changes - It's really hard to test ahead of time to make sure it doesn't break anything. Have you tested this in your fork?

@marbre marbre marked this pull request as draft April 2, 2024 12:36
@marbre
Copy link
Member Author

marbre commented Apr 2, 2024

This seems reasonable of course, but as always with these kinds of changes - It's really hard to test ahead of time to make sure it doesn't break anything. Have you tested this in your fork?

So far I've checked what actions are used within the workflow, but this cannot be considered at testing ahead. I had in mind that this workflow isn't that critical and that it is partly ignored (if I remember one of the pre-merge check discussions correctly). However, I converted this PR to a draft PR and will take a closer look and test within my fork.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with your analysis that this shouldn't fail by changing this, but always good to test. Getting the test setup working can be a little difficult, so if you run into any non-trivial issues, let me now and hopefully I can assist.

@tstellar
Copy link
Collaborator

@marbre Are you planning to commit this?

@marbre
Copy link
Member Author

marbre commented Apr 11, 2024

@marbre Are you planning to commit this?

After @tru's feedback I planned to take a more careful look before commiting this. Unfortunately, I had other things on my list last week and I am currently at EuroLLVM.
However, if you think that we shouldn't care too much if things break by the patch and if it would be okay to fix it afterwards we can already go ahead with this.

@tstellar
Copy link
Collaborator

Now that we are using the pull_request event for the code format job, any changes to it will get tested in the job that is triggered by this PR, so this has already been tested.

@marbre marbre marked this pull request as ready for review April 11, 2024 20:56
@marbre marbre merged commit a952c12 into llvm:main Apr 11, 2024
8 checks passed
@marbre marbre deleted the pr-code-format branch April 11, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants