renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
16.64k stars 2.15k forks source link

Bitbucket Server (maybe Cloud too): default reviewers support #3579

Closed iamstarkov closed 5 years ago

iamstarkov commented 5 years ago

Intro

there is one thing which is missing in Bitbucket Server (Cloud too?): default reviewers.

You can configure them per team(project) or per repository. And then new PRs will have those default reviewers in place implicitly

From the Atlassian wiki:

Default reviewers allow you to automatically add one or more users as reviewers to pull requests. In addition, you can optionally specify how many of the specified default reviewers must approve a pull request prior to merging to ensure that a minimum level of review occurs.

You may have different roles in your team that should be automatically added to pull requests depending on the nature of the pull request. This can be achieved by assigning default reviewers for a specific repository, a specific branch, using a branch pattern, or with a branch type from the branching model. For example, you might specify a release manager be assigned to all pull requests targeting release branches.

Once assigned in the repository settings, default reviewers will be pre-filled during pull request creation. At that time the set of reviewers can then be adjusted for each pull request. — Add default reviewers to pull requests, Bitbucket support


What would you like Renovate to be able to do? I want renovate to take preconfigured default reviewers into account. Cli or config reviewers option doesn't work on scale and through out the time.

Describe the solution you'd like When renovate creates a PR, it checks the default reviewers for the branch it creates PR to, fetch default reviewers for it and assign those.

Describe alternatives you've considered

  1. One options would be to use codeowners and paid plugin, but then you will have to maintain code owners in tens of repos if team changes.
  2. Another options would be to use cli reviewers argument, but it doesnt work on scale for tens of teams with tens of repos each.
  3. 3rd options would be to user repo's renovate config to configure reviewers, but that would prevent smooth on boarding and after on boarding will still have the same drawbacks as the 1st option: you would need to keep renovate configs in each repo in sync with up-to-date team staff.

Additional context Add any other context or screenshots about the feature request here.

iamstarkov commented 5 years ago

It is one feature we are missing at @nordnet. I filed it to request comments and feedback.

rarkins commented 5 years ago

If you configure default reviewers via Bitbucket's interface, doesn't Bitbucket itself add the reviewers once Renovate creates PRs? or maybe I misunderstand what you're hoping for

iamstarkov commented 5 years ago

I thought it would too, but apparently it doesnt. When you create PR via UI, then reviewers are being autosuggersted as in prefilled. I guess Bitbucket uses mentioned endpoint to retrieve the default reviewers and suggest the list before you create the PR manually

iamstarkov commented 5 years ago

I mean if you are open to have a custom logic in the BB server adapter, then I would hop on it when I will get time

rarkins commented 5 years ago

Please tell me if I understand the idea correctly: instead of configuring reviewers in both BitBucket ("default reviewers") as well as Renovate, we add the option that Renovate piggy backs on Bitbucket's configuration? i.e. use their API to learn the default reviewers and then add them to PRs without needing for those usernames to be configured in Renovate also?

iamstarkov commented 5 years ago

@rarkins correct

rarkins commented 5 years ago

Sounds like a good feature. Question is whether to make it opt-in, or opt-out, automatic or manual?

e.g. if no reviewers are configured, should we check with the Bitbucket automatically, or only if opted in?

iamstarkov commented 5 years ago

For us it was an expected behavior and I bet for whoever have their default reviewers configured it would be too. I say this feature should be enabled automatically with an option to opt out

rarkins commented 5 years ago

So:

iamstarkov commented 5 years ago

sounds good

viceice commented 5 years ago

Maybe i can help to implement this 😄

iamstarkov commented 5 years ago

@ViceIce that would be nice

viceice commented 5 years ago

fetching the default reviewers is not a problem, but where to add them to the config?

@rarkins Do you have an idea, where this can be implemented?

rarkins commented 5 years ago

I’d add a new config option like “bbUseDefaultReviewers” of Boolean type set to true by default.

I think the only time we need to add these is when we create a PR so they should be “lazily” fetched only then.

So when creating the PR:

The bbUseDefaultReviewers option should be passed as part on createPR() somthat it’s configurable via packageRules if desired.

viceice commented 5 years ago

If it is a config option, it would be in the local bbs config. so we can check it in createPr.

I will look at it later :-)

viceice commented 5 years ago

github has a similar feature CODEOWNERS.

Should we support /prepare this too? so we only need one config option?

rarkins commented 5 years ago

I think GitHub apply code owners directly to PRs without opting in and without the ability to opt out. So BB seems different in this case - they do it optionally and not conveniently (eg there’s no Boolean option in create PR)

rarkins commented 5 years ago

“Local bbs config” means passed at initRepo? This would work but remove the ability to configure it per-PR

viceice commented 5 years ago

ok, so we have to pass the current config down to createPr

rarkins commented 5 years ago

Yes, an extra parameter to createPR, to be ignored by other platforms. Maybe can be inside an “options” object if we think it’s messy as a direct parameter, although does that make it harder later to TS the function definition?

viceice commented 5 years ago

an options object would be better. no problem for typescript, since we can make it optional.

viceice commented 5 years ago

maybe we should first refactor plattform to typescript before we make bigger changes?

I can do this in multiple small pr.

rarkins commented 5 years ago

I just wasn’t sure if “hiding” platform-specific config inside an “options” object was considered bad practice with Typescript. There’s no problem adding it as a first class function parameter right now

rarkins commented 5 years ago

Actually let’s just pass it in initRepo for now. ie it’s per-repo setting and not per-PR

rarkins commented 5 years ago

Then no change to createPr signature

viceice commented 5 years ago

as the docu states, we can get the default reviewer for a pr, but we have to fetch them in createPr, if bbUseDefaultReviewers is set. I'll try that :-)

viceice commented 5 years ago

should bbUseDefaultReviewers be an admin config? should it be true by default?

rarkins commented 5 years ago

@ViceIce making it an admin config means it's (a) only the bot owner who can decide it, and not the repo owner, and (b) most likely that you'll have the rule across the entire bitbucket server. I thought this might be desirable to configure per-repo instead?

iamstarkov commented 5 years ago

its not per-repo

iamstarkov commented 5 years ago

I mean, you can configure it for repos too, but usually each team has their own project on bitbucket server and then team configures list of default reviewers for the project, which reflects the team members and then each repository in the project inherits project's default reviewers. this way you can maintain default reviewers list for team's repos in one place

viceice commented 5 years ago

You can configure on repo too, if you like.

So if my pr is correct, you can disable within repo.

iamstarkov commented 5 years ago

yes, that's how I would do it too

renovate-bot commented 5 years ago

:tada: This issue has been resolved in version 17.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

ZyanKLee commented 5 years ago

Hello dear gentlemen, I would like to see this great feature to be implemented for the Cloud version as well. Has there been a blocking issue that prohibited you from implementing it for BBC? Else I would ask my colleague to submit the necessary changes.

viceice commented 5 years ago

@ZyanKLee The cloud api is different, so i couldn't simply copy the changes. So feel free to submit a pr for that.