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

Add support for minimum amount of approvals before merge #190

Open
fmvilas opened this issue Oct 24, 2022 · 20 comments
Open

Add support for minimum amount of approvals before merge #190

fmvilas opened this issue Oct 24, 2022 · 20 comments
Labels
area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. enhancement New feature or request good first issue Good for newcomers stale

Comments

@fmvilas
Copy link
Member

fmvilas commented Oct 24, 2022

Have a look at asyncapi/spec#851 (comment).

@fmvilas fmvilas added enhancement New feature or request good first issue Good for newcomers area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. labels Oct 24, 2022
@Amzani
Copy link

Amzani commented Feb 20, 2023

I would like to work on this issue.

@Amzani
Copy link

Amzani commented Mar 3, 2023

@fmvilas @derberg You want to require a minimum of 3 approvals for all AsyncAPI repos or you want to only to do it in some repos like specs ?

cc @KhudaDad414

@KhudaDad414
Copy link
Member

@Amzani I think we only need it to be 3 for specs.
Besides we get the value from an environment variable so each repo can have their own value :)

@Amzani
Copy link

Amzani commented Mar 3, 2023

Great @KhudaDad414 this environment exists already ? or it's something new we have to create ?

@KhudaDad414
Copy link
Member

I don't think it exists yet. we can have a default value of 1 if it doesn't exist in the repo for now.

@fmvilas
Copy link
Member Author

fmvilas commented Mar 6, 2023

Yeah, the environment variable would actually be a secret in the repo: https://docs.github.com/en/actions/security-guides/encrypted-secrets. That's the only way to store info associated with a repo that comes to my mind. If you come up with a more elegant way to solve it, feel free!

@octonawish-akcodes
Copy link

Can I try this.

Copy link
Member

derberg commented Mar 8, 2023

Can I try this.

first we need to figure the solution.


the best if that info is stored in a file in a repo, then any change goes through review.
there is this concept of #137 that we need anyway, not just amount of reviewers but for transparency reasons and increasing size of maintainers best if any repo setting is done through a PR

@Amzani
Copy link

Amzani commented Mar 13, 2023

I like the long term solution to solve this, then we could have 2 options

  1. A central configuration repo (e.g admin) in the AsyncAPI Github organisation that manages the settings for all repos. To implement it we could try something like Safe-settings
  2. Each repo defines its own settings. Something like

If we check the requirements listed in #137, the solutions I've mentioned can:

  • Ensable/Disable discussions
  • Enables only squash and merge on PRs
  • Blocking merges based on policies (number of reviewers)
  • Branches protection configuration
  • Custom configuration (sonarcloud: true or coveralls: true) will be only possible in Safe-settings. the repository-settings doesn't support it, However it can be done by developing our custom app on top of probot and octokit-plugin-config

@fmvilas
Copy link
Member Author

fmvilas commented Mar 13, 2023

I honestly prefer a config file in each repo instead of a central one. This way code owners will have the power to review it and approve it.

@derberg
Copy link
Member

derberg commented Mar 14, 2023

yes, we need to have it per repo definitely (like https://github.com/repository-settings/app I guess), so only codeowners of given repo are involved in review, and they can adjust settings transparently however they need for they repository. Of course if there is a case that there are multiple repos that have exactly the same configuration, we can have it in .github repo and push out to repos that need it using the same automation we have for workflows or code of conduct. I'm referring to https://github.com/asyncapi/.github/blob/master/.github/workflows/global-replicator.yml

On requirements side, user management would be lovely too 😄 Basically anything that requires going to Settings so we can eliminate a need to access Settings in general.

Custom configuration (sonarcloud: true or coveralls: true) will be only possible in Safe-settings. the repository-settings doesn't support it, However it can be done by developing our custom app on top of probot and octokit-plugin-config

For others, like sonar or coveralls, yeah, we definitely will need something custom.

Most important is that we need a strong requirement here to make sure it all works like all else on GitHub Actions. So there are no apps, no deployments and related overhead. Just workflows that support sync of settings whenever they are modified. Maybe solution is to get important code from repository-setting and use in our custom action. Or maybe there is some action out there that we could already use or contribute to.

Afaik with existing mutations in GitHub GraphQL it is not that complicated after all. Updating all stuff with one shot is with one call. So the only complicated part I think is transform of YAML settings to API call.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jul 13, 2023
@aakankshabhende
Copy link

@derberg @Amzani I'm interested in working on this. Could you please assign me this?

@aakankshabhende
Copy link

Also, should the minimum approvals be 3?

Copy link
Member

derberg commented Oct 21, 2024

it is definitely not developed yet

default should be 1 as it is now because of how things work, and per repo can be overwritten.

make sure to first explain how you want to solve it, before you open a PR and hit our review 😃

@aakankshabhende
Copy link

@derberg I believe that we can have a central config file in this repo for minimum approval, then for individual repo we can include a check if the that repository has its own override config (workflow), if not we will enforce the central workflow. I would love to hear any thoughts or ideas on how we can refine this approach.

Copy link
Member

derberg commented Oct 22, 2024

@aakankshabhende please take into account that in AsyncAPI we use https://github.com/derberg/manage-files-in-multiple-repositories action that allows us to push workflows from .github to all the other repositories under asyncapi org. So we can have everywhere the same workflow that will be responsible for this approvals verification. No need for any central workflow.

As if it comes to configuration, instead of the config file, just like in some other issue Fran wrote, we can just have a system variable, that GitHub allows to have. So we have https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables on organization level, and workflow would read this variable, and if someone in some repository wants to overwrite this global default, they just overwrite the variable with custom value only in their repository

@aakankshabhende
Copy link

Got it. Sounds good! Thanks @derberg

@aakankshabhende
Copy link

I'll start working on this

@derberg
Copy link
Member

derberg commented Oct 22, 2024

awesome

please feel free to ask for any clarifications on the way - always better to clarify first than get ping-pong in a PR

if I do not answer here for some reason, feel free to DM me in AsyncAPI Slack

good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. enhancement New feature or request good first issue Good for newcomers stale
Projects
None yet
Development

No branches or pull requests

6 participants