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

Make the governance policy for this repo clear #1856

Merged
merged 4 commits into from Dec 28, 2021
Merged

Make the governance policy for this repo clear #1856

merged 4 commits into from Dec 28, 2021

Conversation

KengoTODA
Copy link
Member

To solve problems caused by our unclear governance policy for this project, create a GOVERNANCE.md at the root directory.
I referred to The nodejs governance policy and the Minimum Viable Governance from GitHub, but the content is made from scratch to enable our small start.

After we merge this change, I will configure the branch protection rule to change the number of necessary approval from 1 to 2. Other automations such as stale action and auto-merge will come next.

@iloveeclipse
Copy link
Member

What is the reason to request two approvals for a PR?

GOVERNANCE.md Outdated Show resolved Hide resolved
@KengoTODA
Copy link
Member Author

What is the reason to request two approvals for a PR?

It is mostly the same to the unwritten rule in this repo. Most of our merged PRs have two approvers, or one approver and another merger.

I personally think that it's a good habit to keep the project secure. We can assume that at least multiple people made judgement on its implementation and maintainability. We can also avoid the situation like "one strong committer" drives everything so no decision is open nor understandable.

ThrawnCA
ThrawnCA previously approved these changes Dec 9, 2021
GOVERNANCE.md Outdated

Both SpotBugs Core Team and SpotBugs user can propose changes to the SpotBugs project via GitHub pull requests. Refer to `.github/CONTRIBUTING.md` to know more detailed requirements for your proposal.

Once a pull request gets two approvals from members of SpotBugs Core Team, pull request will be merged and shipped in the next release. Approver should not be the same as the author(s) of the change.
Copy link
Member

Choose a reason for hiding this comment

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

Approver should not be the same as the author(s) of the change.

This is contradictory to the current state / your comment:

It is mostly the same to the unwritten rule in this repo. Most of our merged PRs have two approvers, or one approver and another merger.

Currently if we have author from committers, we only want one more approval.
With this proposal, we will need two approvals - one more as today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a challenge as noted at https://github.com/orgs/spotbugs/teams/everyone/discussions/9?from_comment=4#discussion-9-comment-4

I think it's reasonable change for governance, how do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. One can't force people to do this if they don't want or can't. I see mostly you and ThrawnCA active, and while I'm still there, I simply have no time to spent on reviews, especially if they are for features I'm not interested personally (lile gradle support etc). So by forcing two additional reviews you force me to do something I won't, and as a result I will be unhappy and feel me bad because on the one side I know your work waits for my review but on the other side I can't afford to spend reasonable amount of time for this (or don't want to review this).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I understood. Thanks. I will remove the limitation later.

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

I think I'm OK in general, few style/language issues.

GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
GOVERNANCE.md Outdated Show resolved Hide resolved
@KengoTODA
Copy link
Member Author

Thanks @iloveeclipse, I've reflected your review to the doc. It helped me a lot to make it better! :)

@sonarcloud
Copy link

sonarcloud bot commented Dec 22, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@KengoTODA KengoTODA merged commit 1f72232 into master Dec 28, 2021
@KengoTODA KengoTODA deleted the governance branch December 28, 2021 01:21
KengoTODA added a commit that referenced this pull request Dec 28, 2021
hazendaz pushed a commit to hazendaz/spotbugs that referenced this pull request Sep 30, 2023
hazendaz pushed a commit that referenced this pull request Oct 7, 2023
hazendaz pushed a commit that referenced this pull request Dec 17, 2023
hazendaz pushed a commit that referenced this pull request Feb 3, 2024
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

3 participants