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

[IMP] make APPROVALS_REQUIRED configurable #107

Merged
merged 3 commits into from
Mar 28, 2020

Conversation

legalsylvain
Copy link
Collaborator

@legalsylvain legalsylvain commented Mar 26, 2020

For the time being, APPROVALS_REQUIRED is hardcoded with the value 2.

With that PR, it is configurable.

Note : It is not the perfect design, as the approvals number is dynamic, in the real (OCA) life, and depends on the state of the modules. (stable / Beta / etc...) but it is a little step forward.

Make also MIN_PR_AGE configurable.

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can you add the newsfragment? 107.feature in this case.

Suggestion: "Add APPROVALS_REQUIRED and MIN_PR_AGE configuration options to control the conditions to set the Approved label."

@legalsylvain
Copy link
Collaborator Author

Ok ! Thanks for the review ! A little out of topic but I was thinking to the interest to implement two levels if the approval is green (comes from maintainers of the repo) or white (comes from any people) do you think it's relevant ? Usefull for the OCA ?

@sbidoul
Copy link
Member

sbidoul commented Mar 28, 2020

two levels if the approval is green (comes from maintainers of the repo) or white (comes from any people)

Could be useful yes. To determine who is a green approver, we could use user_can_push from #71. A separate PR could first refactor _user_can_merge into user_can_push.

@legalsylvain legalsylvain force-pushed the IMP-APPROVALS_REQUIRED-setting branch from 56d948c to 93cb9fa Compare March 28, 2020 10:16
@legalsylvain
Copy link
Collaborator Author

Hi @sbidoul

  • fragment added.
  • I will do another PR for adding a MAINTAINERS_APPROVALS_REQUIRED coming soon. you can proceed with this one, if you think all is OK.

@sbidoul sbidoul merged commit 6aaac47 into OCA:master Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants