-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add PR labeler based on LoC #50
Conversation
Hmm looks like there is some permission issue for the Action app to access this repo:
After a quick research, it looks like GitHub Action's token will be downgraded to read-only in PRs from forked repos. |
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.
Hi @dilyar85,
Thank you for owning and driving this! I really appreciate it! In addition to the feedback I have already provided, I would also like you to update all of the size labels to exclude:
- "*_generated.go" # generated Go sources
- "config/crd/bases/.*" # generated CRDs
- "docs/apis/v*.md" # generated API documentation,
# ex. docs/apis/v1alpha1.md
The problem is that the labeler action does not currently support an exclude option. There is an issue from November 2021 requesting this feature -- srvaroa/labeler#33. Apparently the following does not work:
files:
- "^*_generated.go" # generated Go sources
- "^config/crd/bases/.*" # generated CRDs
- "^docs/apis/v*.md" # generated API documentation,
# ex. docs/apis/v1alpha1.md
The standard GitHub labeler action does support exclusions -- actions/labeler#22. However, it does not support labeling based on the size of the diff, although an enhancement request for this was filed last week -- actions/labeler#486.
For now I think we go with srvaroa/labeler@v0.9 and acknowledge that the generated files cannot be easily ignored. I'm not sure if it's worth maintaining a list of all the files to include -- that will inevitably miss something as files are added/removed.
Please update the labeler action workflow YAML with some comments/notes regarding the concerns in this feedback so that others can track that we had this discussion. Thanks!
Hi @dilyar85, Let's chat about this before you do anything. I already mentioned that you'll need to validate this in your fork. The reason is the perms. There is a reason https://github.com/vmware-tanzu/vm-operator/blob/main/.github/workflows/post-coverage-to-pr.yml exists the way it does -- actions spawned as a result of a PR from a fork do not have permission to write to the PR on the primary repo. Thus the However, in the case of this workflow, you should merge it to main in your fork. That's what I did to get the workflows correct for this repo in the first place. I kept rebasing main in my fork to validate the configurations. Then I would open PRs against my fork to validate PR workflows, etc. Once I was happy with everything, I would then open a PR against the primary repo from a branch on my fork, ensuring my fork's |
Minimum allowed line rate is |
@akutz Thanks for the review. I have updated this change to address your comments above. |
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.
Lgtm!
This patch adds a new GitHub action that automatically attaches labels to PRs based on their LoC.
It can also be served as a reminder for developers to conduct necessary testing for PRs with large LoC.
Testing Done:
main
branch with this change pushedScreenshot of the above PR in my fork: