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 Merging reviewers? #5121

Closed
straub opened this issue Jan 9, 2020 · 4 comments · Fixed by #5152
Closed

Allow Merging reviewers? #5121

straub opened this issue Jan 9, 2020 · 4 comments · Fixed by #5152
Assignees
Labels
priority-4-low Low priority, unlikely to be done unless it becomes important to more people type:feature Feature (new functionality)

Comments

@straub
Copy link
Contributor

straub commented Jan 9, 2020

What would you like Renovate to be able to do?
I'd like to be able to merge reviewers arrays across preset configs.

For example:

{ "extends": [":reviewer(foo)", ":reviewer(bar)"] }

I'd expect both reviewers to be added, but instead "bar" seems to displace "foo" as the sole reviewer.

Describe the solution you'd like
The reviewers option could be flagged mergeable in its definition.

Describe alternatives you've considered
Individual repo configs that want to add additional focused reviewers have to repeat default reviewers normally provided by our custom preset, which makes managing that list of default users more difficult.

If I'm overlooking a better solution, please let me know!

Additional context
If this is not an intentional omission, I'd be very much open to attempting a PR for it!

@JamieMagee JamieMagee added the type:feature Feature (new functionality) label Jan 9, 2020
@rarkins
Copy link
Collaborator

rarkins commented Jan 10, 2020

@straub the "problem" with merging the reviewer field is that it then wouldn't allow you to ever override reviewers. Within Renovate users it's common that people define a default reviewer at the top level but want an alternative reviewer or reviewers for specific files or packages (similar to your use case, but they don't want the original reviewer included). If Renovate merged reviewers by default then it would mean you couldn't remove that top-level reviewer.

In your case, can you do the following instead? i.e. a non-preset approach:

{ "reviewers": ["foo", "bar"] }

@straub
Copy link
Contributor Author

straub commented Jan 10, 2020

@rarkins Thanks for your reply!

I had a hunch it was something like that :) I can definitely do what you've described, but I'm searching for a better process for reviewer management because my "foo" is really a list of several base reviewers and I have more than 60 repos and variations on "bar". When the "foo" base reviewer list needs to change, there are many configs to find and update.

Would you be open to a PR for a new config option to support this use-case? I'm imagining something like baseReviewers or additionalReviewers, depending on the approach you prefer, that could be mergeable while leaving the existing behavior of reviewers as-is. I self-host, so a single overall option to make reviewers itself behave as mergeable could also solve for this.

Really appreciate your time, both helping me here and supporting this amazing tool.

@rarkins
Copy link
Collaborator

rarkins commented Jan 10, 2020

@straub I'm definitely ok with the idea of supporting mergeable reviewers - we just need to work out the syntax. I prefer to leave the existing name (reviewers) and functionality (replacing) but then have a new field additionalReviewers like you suggest. additionalReviewers can be defined as mergeable and at the time of PR creation we'll combine and deduplicate reviewers + additionalReviewers for the final array. We should also make sure these fields are adequately documented to try to minimize confusion.

@rarkins rarkins added priority-4-low Low priority, unlikely to be done unless it becomes important to more people ready labels Jan 10, 2020
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 19.97.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-4-low Low priority, unlikely to be done unless it becomes important to more people type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants