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

Master branch protection #20

Open
BelfordZ opened this issue Apr 3, 2019 · 4 comments
Open

Master branch protection #20

BelfordZ opened this issue Apr 3, 2019 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@BelfordZ
Copy link
Contributor

BelfordZ commented Apr 3, 2019

I've tagged this issue as a bug since having an unprotected master branch is universally a bad idea. This allows people to accidentally do very bad things, and as such, should be considered a bug before it manifests as an actual catastrophic issue.

I propose the following configuration for protecting the master branch:

image

I will go ahead and apply the protection rule now. In the comments of this issue.

With that, I propose that this issue the be place we discuss any changes to this base configuration.

@BelfordZ BelfordZ added the bug Something isn't working label Apr 3, 2019
@BelfordZ BelfordZ self-assigned this Apr 3, 2019
@soc1c
Copy link
Contributor

soc1c commented Apr 3, 2019

I'm all for it but your settings will prevent us from merging anything.

Following suggestions:

  • 1 review is sufficient for ECIPs, maybe 2 if you insist. 3 will basically block the entire process here.
  • remove code owners requirement. that's usually not applicable in ECIPs repo.
  • we don't have CI, so no need to tick that box, please remove status checks or add CI.

I'm cool with signed commits and excluding administorators 😎

@eyfl
Copy link
Contributor

eyfl commented Apr 4, 2019

I am ok with 3 review, since this is for ECIP
And I don't think we need CI or the status check here either

@BelfordZ
Copy link
Contributor Author

BelfordZ commented Apr 4, 2019

1 review is sufficient for ECIPs, maybe 2 if you insist. 3 will basically block the entire process here.

1 is insufficient. 1 person should not be the sole editor of this repo.

We don't need to move quickly on merging PRs in this repo. I'd argue quite the opposite, and be happy to leave pull requests up for however long it takes to get 3 people to look at it.

Increasing the number of people required to approve will only serve to increase the quality of each PR.

remove code owners requirement. that's usually not applicable in ECIPs repo.

willdo.

we don't have CI, so no need to tick that box, please remove status checks or add CI.

The hope is that we will add some checks in place. We have a couple of tools we are using on open-rpc spec that we can borrow over here too. Also, we should have a tool for validating that it adheres to the ECIP format. Since there are no status checks ATM, it shouldn't be blocking or affect anything, so I'll turn them off until we add some.

@BelfordZ
Copy link
Contributor Author

BelfordZ commented Jun 5, 2019

Anyone have thoughts on dropping the requirement for signed commits? Its currently blocking this pr: #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants