-
Notifications
You must be signed in to change notification settings - Fork 508
Added the split method for labeler workflow #788
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
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/color-labels.yml
Outdated
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| const labels = [ | ||
| { name: 'cli', color: '0E8A16', description: 'Changes to CLI components' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of trying to encode the different label categories into the workflow job itself, maybe instead we could use the PR number parameter to actions/labeler in the label-apply workflow like this:
- uses: actions/labeler@v6
with:
pr-number: |
1
<PR number we got from the artifact download>
We should double check that the labeler doesn't try to read the untrusted code. But my understanding is that the action does not checkout the PR code, just analyzes based on the files that have been changed.
Note: I think it would be unsafe to read the labeler's configuration file from the PR, we might have to settle for using the configuration in main. But we should look into that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, Katie! I've updated the workflows to use actions/labeler@v5 with the PR number from the artifact as you recommended.
.github/workflows/color-labels.yml
Outdated
| permissions: | ||
| issues: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #781, the top level permissions should always be the least permissive possible and any additional permissions should be set at the job level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the permissiion to read only
…atie's security recommendations
|
@katiewasnothere can you look at the changes I made please let me know if they are good |
.github/workflows/color-labels.yml
Outdated
| @@ -0,0 +1,65 @@ | |||
| name: Color Labels | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this. We can make them by hand for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i remvoe the colors file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
| with: | ||
| pr-number: ${{ steps.pr-number.outputs.number }} | ||
| repo-token: ${{ secrets.GITHUB_TOKEN }} | ||
| configuration-path: .github/labeler.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have checked out the repo to access this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to checkout the repository to access the .github/labeler.yml configuration file. Without checkout, the workflow runner won't have access to any files from the repo, including the labeler configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the repo being checked out in this workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I accidentally removed it let me add it again. Thank you Katie for pointing that out.
Type of Change
Motivation and Context
This can basically Help teams know what changes are made in pr
Testing
Structure how workflow runs