-
Notifications
You must be signed in to change notification settings - Fork 132
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
feat(blunderbuss): assign_prs_always in config #5051
Conversation
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.
Left a suggestion, let me know what you think.
Also, whatever we decide on, mind adding to packages/blunderbuss/README
.
- java-samples-reviewers | ||
assign_prs: | ||
- prs1 | ||
assign_prs_always: |
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.
Rather than introducing an additional assign_prs_always
which seems a little redundant given that we have assign_prs
, what if you instead introduce:
assignment_config:
apply_all_pr_rules: true
Or something like this.
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.
If apply_all_pr_rules
was set to true
, it implies we should apply both assign_prs_by
and assign_prs
.
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 prefer @amanda-tarafa's proposal. What does @amanda-tarafa say?
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 also prefer my own proposal (haha) because it means that assign_prs can act as a fallback for assign_prs_by regardless of whether assign_prs_always is set. For instance, for products whose samples don't have an owner (i.e. we don't have an assign_prs_by), we still need two reviews, so we still need two assignees. We would get one from assign_prs (as a fallback for assign_prs_by) and one from assign_prs_always.
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 prefer my proposal, but am outvoted 😝 . Approved!
Fixes #5043