Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs: Added clear community contribution guidelines #504

Merged
merged 7 commits into from
Feb 4, 2022

Conversation

AlmasB
Copy link
Collaborator

@AlmasB AlmasB commented Jan 27, 2022

This PR defines and clarifies the contribution workflow (which has been implicit so far).

Practically, the only change is that new PRs must follow the (outlined) subset of Conventional Commits to improve automated changelog generation and quality of text for users. The recently merged #458 introduced jreleaser, which is already configured to read this new format of PR names (commits to master).

If contributors forget or omit this, maintainers will still be able to change the PR name before the merge. Alternatively, if you do not wish to have this extra responsibility, simply ping me after a PR is approved and I'm happy to rename it as appropriate.

This PR itself is the first to follow the new workflow, so pinging all @johanvos @eugener @Oliver-Loeffler @abhinayagarwal @jperedadnr for information / comments / approval.

Issue

Fixes #451

Progress

@Oliver-Loeffler
Copy link
Collaborator

For me this process looks very straight forward. I consider this as extremely helpful that things are laid out and one can follow this guide, this will especially help new contributors. Also, the PR naming schema is actually okay for me - can imagine that this really helps a maintainer to structurize work.

We should listen to contributor feedback on these rules/guidelines - curious how the feedback will be.
Very good proposal @AlmasB.

CONTRIBUTING.md Outdated
3. A maintainer will confirm the issue is valid and can be assigned to you for a fix.
4. You produce a Pull Request following the Standards below.
5. Once you tick all check boxes on the Pull Request template, it will be reviewed by a maintainer or a community member.
6. Once the Pull Request has at least one approval, it will be ready for a merge.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some PR's require > 1 approval.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will update this guidance once we define / agree #455 and therefore define the difference between minor and major, or whatever names we agree. Based on that, we can then agree that minor needs at least 1 approval, major needs at least n, whatever n is.

@johanvos
Copy link
Contributor

I think it's a good approach to discuss.
Personally, I'm very impressed by the Skara tools and the way they are applied to https://github.com/openjdk/jdk and https://github.com/openjdk/fx .
Skara automates lots of checks and communication, and makes sure PR's are not accidentally merged etc.
We don't have to use this though, I'm just mentioning it as a great resource.

To me, the most important part of a PR is that the author can indicate how the code in the PR is expected to be maintained. It's easy to change code, but it's hard to maintain code. This is something that is very hard to define in rules, though, so I think we should really emphasize this.

@AlmasB
Copy link
Collaborator Author

AlmasB commented Jan 27, 2022

This is something that is very hard to define in rules, though, so I think we should really emphasize this.

I'm bit unsure about how we can emphasise this. Do you mean contributors should provide tests for their code, which might be quite a reasonable way to address this?

@johanvos
Copy link
Contributor

It is more general. As a contributor, you should write your code in such a way that others can maintain it. The biggest issues we often see in large projects is where something should change, but nobody knows exactly why it was there in the first place, or what the impact will be when it is changed.
Tests are definitely a good requirement for this.

@eugener
Copy link
Contributor

eugener commented Jan 28, 2022

Looks good, but I think we need some verbiage related to submission of new features.
Creating an issue might not be the best approach. What about asking contributors to start a discussion first?
A discussion can easily be converted to an issue using GitHub UI, if enough people are interested.

@AlmasB
Copy link
Collaborator Author

AlmasB commented Jan 28, 2022

@eugener good point. I'll ponder the process for new feature submission and perhaps add a very brief section on "New Feature Proposal Workflow", which can possibly include #488.

@AlmasB
Copy link
Collaborator Author

AlmasB commented Feb 1, 2022

I've had a go at:

  • @abhinayagarwal semantic.yml with the appropriate subset of CC
  • @johanvos added a note on tests and the number of approvals for certain PRs/issues
  • @eugener added a workflow for feature requests to start with a discussion

Please check when available.

@eugener
Copy link
Contributor

eugener commented Feb 1, 2022

LGTM. 2 more things I can think of..

  1. Should PRs be squashed instead of merged for cleaner main line? If this is something we want - we need to describe it in the documentation and configure the repo for it.
  2. Should repository branch protection be configured to enforce number of reviewers and PR checks?

@AlmasB
Copy link
Collaborator Author

AlmasB commented Feb 3, 2022

  1. PRs are squashed by default and the merge commit is disabled for this repo, so it is enforced by the settings. I will update the doc to mention this.
  2. Good idea, @abhinayagarwal do you have access to repo settings? If so, please can you enable branch protection to require at least 1 approval (common denominator), so this is enforced by the settings. The docs already reflect this.

@eugener
Copy link
Contributor

eugener commented Feb 3, 2022

Had some time and updated the branch protection settings. For ALL (*) branches:

  • Require a pull request before merging
  • Require min 1 approval
  • Dismiss stale pull request approvals when new commits are pushed
  • Require review from Code Owners

Hopefully this is not too restrictive. Would also love to include status checks on PRs.
@aalmiray Can any steps from existing workflows be used or new workflow is needed for this?

@aalmiray
Copy link
Collaborator

aalmiray commented Feb 3, 2022

Those checks would have to be added, some as githooks, some through the GH UI, and some perhaps as additional steps on existing workflows.

@eugener
Copy link
Contributor

eugener commented Feb 3, 2022

At the minimum, multi-platform builds and tests have to pass.
Can also add CodeQL scans

@AlmasB
Copy link
Collaborator Author

AlmasB commented Feb 4, 2022

In the meantime, to expedite the process, how about we just restore the old multiplatform build.yml but:

  • remove on master, so just leavepull_request
  • remove draft release, as we now have dedicated workflows

@eugener
Copy link
Contributor

eugener commented Feb 4, 2022

That should work IMO - will send a PR within few hours.
Once it's merged, I will update repo's branch protection settings.

@eugener eugener mentioned this pull request Feb 4, 2022
3 tasks
@eugener
Copy link
Contributor

eugener commented Feb 4, 2022

@AlmasB See the PR #513. The newly updated checks are already working there.

@eugener
Copy link
Contributor

eugener commented Feb 4, 2022

Branch protection rule is updated now ..
image

@AlmasB
Copy link
Collaborator Author

AlmasB commented Feb 4, 2022

Looks good, in which case this PR should be ready to go too I think.

@eugener
Copy link
Contributor

eugener commented Feb 4, 2022

I'm good with it

@AlmasB AlmasB requested a review from eugener February 4, 2022 17:53
@AlmasB AlmasB merged commit bb92623 into gluonhq:master Feb 4, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardise commit messages
6 participants