Skip to content

feat(project-repo): standardize github settings and commit types #45

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.

Sign up for GitHub

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

Open
d-loose wants to merge 2 commits into
base: main
Choose a base branch
from
+29 −15
Open

feat(project-repo): standardize github settings and commit types #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e8aadf
reduce the number of conventional commit types
d-loose Nov 13, 2024
1f74ccd
add proposed github settings and adjust merge flow accordingly
d-loose Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions project-repo/github-settings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Recommended GitHub repository settings

In order to encourage all contributors to follow the recommended [review process](review-process.md), the repository settings listed in the following sections should be applied.

## Pull requests

The settings listed below should be enabled/disabled as indicated. All other settings can be chosen based on project-specific needs.

- [x] Allow merge commits (default commit message: "Pull request title and description")
- [ ] Allow squash merging
- [ ] Allow rebase merging
Copy link
Contributor

@3v1n0 3v1n0 Nov 18, 2024

Choose a reason for hiding this comment

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

Mh, personally in some projects I'd prefer to allow using a linear history (we do it already in various ubuntu/* projects that has various benefits (mostly for bisecting and testability of each commit). Thus this option can be useful in those contexts.


- [x] Automatically delete head branches


## Branch protection rules

The `main` branch (and any other public development branches from which releases are created) should be protected with by a corresponding set of rules.
The settings listed below should be enabled/disabled as indicated. All other settings can be chosen based on project-specific needs.

- [x] Require a pull request before merging
- [x] Require approvals
- [x] Dismiss stale pull request approvals when new commits are pushed
Copy link
Contributor

@didrocks didrocks Nov 14, 2024

Choose a reason for hiding this comment

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

That’s the part incompatible with the workflow itself. If we allow both, maybe we should set that to optional and add a note for it? But I think this depends on the discussion below :)

- [x] Require status checks to pass before merging
- [x] Require conversation resolution before merging
- [x] Require signed commits
18 changes: 3 additions & 15 deletions project-repo/review-process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,17 @@ For this, the PR title should be of the form: `<type>(<scope>): <subject>`. The

The accepted `types` are:

* `build`: Changes that affect the build system or external dependencies
* `ci`: Changes to the CI configuration
* `deps`: Update dependencies
* `docs`: Documentation only changes
* `feat`: A new feature
* `fix`: A bug fix
* `maint`: Changes that do not affect the meaning of the code (white-spaces, formatting, missing semi-colons, typo fixes, etc). Gardening…
* `perf`: A code change that improves performance
* `refactor`: A code change that neither fixes a bug nor adds a feature (nor add bugs)!
* `tests`: Adding missing tests or correcting existing tests

> TODO: have a document defining what to configure in every new GitHub project (webhook for Jira, merge commit + default commit template, automated merged branch deletion).
* `misc`: A miscellaneous change that doesn't fit any of the other cases, e.g. build system or CI changes, dependency updates, code formatting changes, or changes to the test suite. Providing a `scope` is mandatory.

## Process

Don't hesitate to comment and reiterate! When you engage with a pull request, the expectation is that you remain responsive, and not let the pull request go stale.

1. Team members should [sign their commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits). While this may be difficult to enforce for community contributions, we expect team members to _always_ follow this guideline.
1. Team members should [sign their commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits). If community contributors are unable to sign their commits for any reason, the reviewer should sign their commits before merging.
1. The pull request should always be opened as a draft, to avoid notifying the reviewers before the tests pass and the pull request is fully ready for review. Ensure the description of changes is readable and understandable by prospective reviewers. The pull request should be linked to a Jira card, using `UDENG-XXX` in its description, so that Jira links against it.
1. The pull request diff should be examined by the submitter - it may happen that a debug print statement was left in, or a non-related change was accidentally `git add`ed. Please at least do a quick visual scan to help catch such potential issues/typos.
1. The submitter should wait and check for the CI feedback to indicate a pass (linting, testing, building, automated generation, etc.). Static security check tool (part of linting if possible) is part of that stage. Feel free to ping a team member to ask for help before a formal review, if something doesn’t pass (like tests) and you don’t have any ideas on how to fix it. In such cases, the pull request description should mention that the tests don’t pass.
Expand All @@ -77,12 +70,7 @@ Don't hesitate to comment and reiterate! When you engage with a pull request, th
> It’s always the person opening a comment, **not** the submitter of the pull request, that should resolve the comments, once they are happy with the particular change.

This cycle is repeated if there are further comments or if some are still left open. Once all comments are resolved, the reviewer should approve.

### Final merge

1. The submitter will squash the optional additional commits and force push to the pull request.
1. The submitter examines one last time the diff, the test results, and the coverage.
1. The submitter is in charge of merging the changes by clicking the merge button.
Finally, the submitter should merge the changes by clicking the merge button.
Copy link
Contributor

@didrocks didrocks Nov 14, 2024

Choose a reason for hiding this comment

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

I am with you on this, but some team member rely on squashing to rewrite history before merging so much, and we discussed that back and forth. Maybe we can advise a default workflow, but then, if the reviewer wants to resquash, still trust them? So far, from memory, we only had one booboo in all our PRs during rebasing… (and I am not even sure if a last finale look would have caught it)


> Note: The final merge steps may be proceeded by the reviewer if we are in a hurry. However, it’s preferable to allow the submitter to do the actual merging, as an additional way of acknowledging their work and enjoying the dopamine boost. :-)

Expand Down