From 570c23784bf8721e7617df03b2d68e740b8c143c Mon Sep 17 00:00:00 2001 From: Jennifer Q <66472237+latin-panda@users.noreply.github.com> Date: Fri, 30 Aug 2024 23:00:26 +0700 Subject: [PATCH] chore: clarify pull request process (#1500) Co-authored-by: Andra Blaj --- content/en/contribute/code/workflow.md | 191 ++++++++++++++----------- 1 file changed, 108 insertions(+), 83 deletions(-) diff --git a/content/en/contribute/code/workflow.md b/content/en/contribute/code/workflow.md index 8f9eaa4d4..2803fac85 100644 --- a/content/en/contribute/code/workflow.md +++ b/content/en/contribute/code/workflow.md @@ -106,77 +106,6 @@ The CHT has a [fully automated end-to-end testing suite](https://github.com/medi From time to time QA Engineers will perform smoke tests, scalability tests, performance tests, and penetration tests to pick up on gradual regressions that may have crept in. The ultimate goal is that these tests will eventually be automated and added to the CI suite as well. -### Migrating - -When the schema is changed you must also provide a migration so when instances are upgraded existing data is compatible with the new code. - -## Commits - -The main branch is `main` (or `master`) which must be kept stable so as not to impact other developers and so a release branch can be created as needed. To achieve this (almost) all development should be done in a branch and submitted via a PR for code review. This means the CI runs and another developer has signed off on the change before it hits the `main` branch. - -### Commit message format - -The commit format should follow this [conventional-changelog angular preset](https://github.com/conventional-changelog/conventional-changelog/tree/master/packages/conventional-changelog-angular). This means that some of the release process can be automated. See the list of commit types and examples below: - -Type | Description | Example commit message | Release type --- | -- | -- | -- -Bug fixes | Change code that wasn't working as intended. | fix(#123): infinite spinner when clicking contacts tab twice | patch -Performance | A code change that improves performance. Measure the performance improvement to inform the community. | perf(#789): lazily loaded angular modules | patch -Features | A new feature or improvement that users will notice. | feat(#456): add home tab | minor -Non-code | A change that user won't notice, like a change in a README file, adding e2e tests, updating dependencies, removing unused code, etc. | chore(#123): update README | none - -{{% alert title="Note" %}} -Breaking changes should be explained under the commit type (feat, fix and perf) using the prefix `BREAKING CHANGE`. -Consider the following example: - -``` - perf(#2): remove reporting rates feature - BREAKING CHANGE: reporting rates no longer supported -``` - -Any other further information should be provided in the second line of the commit message, respecting 79 character line widths. Using `git commit -v` is recommended to review your diff while you write your commit message. -{{% /alert %}} - -See tips on [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/) and add your favorites here. - -Never force push remote. Prefer rebasing over merging as it makes for a cleaner history. - -Commit reformats and refactors separately from actual code changes to make reviewing easier. - -Read more about [using git](https://git-scm.com/doc/ext). - -## Branches - -- The main branch is `main` or `master` and is the github default branch and contains the latest code. -- Release branches have the form `..x` and should be stable. -- Feature branches have the form `-` and are work in progress. - -{{% alert title="Note" %}} When backporting changes to an earlier release branch you should `git cherry-pick` the appropriate commit(s) from the main branch into the release branch. Then use a pull request to make sure tests pass on CI before merging (you do not need to get the pull request approved if there were no conflicts when cherry-picking). {{% /alert %}} - -## Issues - -Issues are managed in Github. Issues should be created in the repository where the changes need to be made. If it is not clear in which repo to open an issue the default should be the `cht-core` repository. If it is a security or sensitive issue it should be opened in the private `medic-projects` repository. - -When creating issues add the appropriate [Priority](https://github.com/medic/medic/labels?utf8=%E2%9C%93&q=Priority%3A+) and [Type](https://github.com/medic/medic/labels?utf8=%E2%9C%93&q=Type%3A+) labels. - -### Regressions - -When a bug is found that impacts functionality that worked in a previous version, it's important that these are labelled properly so someone who is planning to upgrade can find it. To flag this, add the "Regression" label, and a labels in the form "Affects: {{version}}" (e.g.: "Affects: 3.14.0") for each version where this bug exists. It's likely that the label for this specific version doesn't exist so you may have to create it. This ensures that issue is listed as a Known Issue in the Release Notes for that version. - -## Project States - -When the issue is scheduled for development it will be added to the [Product Team Activities project](https://github.com/orgs/medic/projects/134). Each column in the project represents the state the issue is in. - -### Todo - -Issues in this column have been prioritised and are ready for development. The issue has all the detail needed to begin development and it is free for anyone to start work on. If you start work on an issue, assign it to yourself and move it to "In progress". - -### In progress - -Issues in this column are being actively worked on, which includes development, design, code reviews, and testing. - -Any code should be in a branch in each of the repositories you update. The name of the branch should be in the form `-`, for example `1104-inclusive-export`. Follow the [Quality Assistance]({{< ref "contribute/medic/product-development-process/quality-assistance" >}}) process to take full ownership of what you are building. - Use the following template for QA feedback throughout the development. {{< tabpane persist=false lang="markdown">}} @@ -251,22 +180,118 @@ The ticket needs further development. {{< /tab >}} {{< /tabpane >}} -A great way to facilitate discussion and collaboration is with a Draft PR. +### Migrating + +When the schema is changed you must also provide a migration so when instances are upgraded existing data is compatible with the new code. + +## Commits + +The main branch is `main` (or `master`) which must be kept stable so as not to impact other developers and so a release branch can be created as needed. To achieve this (almost) all development should be done in a branch and submitted via a PR for code review. This means the CI runs and another developer has signed off on the change before it hits the `main` branch. + +### Commit message format + +The commit format should follow this [conventional-changelog angular preset](https://github.com/conventional-changelog/conventional-changelog/tree/master/packages/conventional-changelog-angular). This means that some of the release process can be automated. See the list of commit types and examples below: + +Type | Description | Example commit message | Release type +-- | -- | -- | -- +Bug fixes | Change code that wasn't working as intended. | fix(#123): infinite spinner when clicking contacts tab twice | patch +Performance | A code change that improves performance. Measure the performance improvement to inform the community. | perf(#789): lazily loaded angular modules | patch +Features | A new feature or improvement that users will notice. | feat(#456): add home tab | minor +Non-code | A change that user won't notice, like a change in a README file, adding e2e tests, updating dependencies, removing unused code, etc. | chore(#123): update README | none + +{{% alert title="Note" %}} +Breaking changes should be explained under the commit type (feat, fix and perf) using the prefix `BREAKING CHANGE`. +Consider the following example: + +``` + perf(#2): remove reporting rates feature + BREAKING CHANGE: reporting rates no longer supported +``` + +Any other further information should be provided in the second line of the commit message, respecting 79 character line widths. Using `git commit -v` is recommended to review your diff while you write your commit message. +{{% /alert %}} + +See tips on [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/) and add your favorites here. + +Never force push remote. Prefer rebasing over merging as it makes for a cleaner history. + +Commit reformats and refactors separately from actual code changes to make reviewing easier. + +Read more about [using git](https://git-scm.com/doc/ext). + +## Branches + +- The main branch is `main` or `master` and is the github default branch and contains the latest code. +- Release branches have the form `..x` and should be stable. +- Feature branches have the form `-` and are work in progress. + +{{% alert title="Note" %}} When backporting changes to an earlier release branch you should `git cherry-pick` the appropriate commit(s) from the main branch into the release branch. Then use a pull request to make sure tests pass on CI before merging (you do not need to get the pull request approved if there were no conflicts when cherry-picking). {{% /alert %}} + +## Issues + +Issues are managed in Github. Issues should be created in the repository where the changes need to be made. If it is not clear in which repo to open an issue the default should be the `cht-core` repository. If it is a security or sensitive issue it should be opened in the private `medic-projects` repository. + +When creating issues add the appropriate [Priority](https://github.com/medic/medic/labels?utf8=%E2%9C%93&q=Priority%3A+) and [Type](https://github.com/medic/medic/labels?utf8=%E2%9C%93&q=Type%3A+) labels. + +### Regressions + +When a bug is found that impacts functionality that worked in a previous version, it's important that these are labelled properly so someone who is planning to upgrade can find it. To flag this, add the "Regression" label, and a labels in the form "Affects: {{version}}" (e.g.: "Affects: 3.14.0") for each version where this bug exists. It's likely that the label for this specific version doesn't exist so you may have to create it. This ensures that issue is listed as a Known Issue in the Release Notes for that version. + +## Project States + +When the issue is scheduled for development it will be added to the [Product Team Activities project](https://github.com/orgs/medic/projects/134). Each column in the project represents the state the issue is in. + +### Todo + +Issues in this column have been prioritised and are ready for development. The issue has all the detail needed to begin development and it is free for anyone to start work on. If you start work on an issue, assign it to yourself and move it to "In progress". + +### In progress + +Issues in this column are being actively worked on, which includes development, design, documentation, code reviews, and testing. + +#### Quality Assistance + +Involve [Quality Assistance]({{< ref "contribute/medic/product-development-process/quality-assistance" >}}) as early as possible in the development process so you can work together on a testing strategy for that specific issue. + +#### Create a branch + +Create a branch following the [guideline]({{< ref "#branches" >}}) and push [commits]({{< ref "#commits" >}}) at least once a day to a remote repository. This ensures that the code is always backed up and safe, protects against accidental deletes, and allows team members to see the latest changes and work together more effectively. + +#### Pull requests + +Create a Draft Pull Request to facilitate discussion and collaboration with quality assistance engineers and developers. -Once you're confident that the change is complete and ready to be merged: +Once you are confident that the change is complete and ready to be merged: -1. Submit a PR for each of the repositories. Each PR message and description will become the commit message and description so keep the message concise, describe what and why rather than how, and link to the issue in the description (eg: "medic/cht-core#123"). -1. Wait for the builds to succeed and ensure there are no conflicts with the the main branch so the PR can be merged. -1. Pick one Reviewer for the PR and work with them until the code passes review. In some special cases more than one Reviewer may be necessary, but be specific about additional Reviewers and ensure you really need each of their additional reviews for a good reason. Remember, anyone can collaborate on PRs even if they aren't an official Reviewer. If you add a QA Engineer as a Reviewer, briefly comment in the ticket about what kind of testing review you expect from that engineer. +1. Change the Pull Request from `Draft` to `Ready for review`. +2. The Pull Request title will be the commit message, it is important to follow the [commit message format]({{< ref "#commit-message-format" >}}) to name the Pull Request title properly. +3. Add a Pull Request description: -Once all PRs have been approved: + - Add a description of changes, decisions, backstory, thinking process, and any extra information to facilitate the review process and reduce follow-ups. + - Add videos or screenshots of the tests you did before submitting the Pull Request. This increases understanding of the work and allows the reviewers to catch any uncovered case. + - Add the issue number, for example: `medic/cht-core#123`. -1. Write a useful commit message in the PR using the [commit message format]({{< ref "#commit-message-format" >}}). -2. Click the button to "Squash and Merge" the PR. -3. If a backport is required cherry-pick the merged commit back to the release branches it's required in. -4. Ensure the issue is added to the appropriate release milestone, which is the earliest semver version the change will be released in. This ensures it will be included in the release notes. -5. Once all PRs have been merged, close the issue. This will automatically move it to "Done". +4. Do a self-code review before asking for a review. This is a good practice, as you will almost always find things to fix. It saves a lot of time for you and the reviewers. +5. Pick one reviewer for the PR and work with them until the code passes review. It is okay to include one additional reviewer who has more experience in a particular subject. + + - Coordinate with a QA engineer and add them as reviewers when you need specific quality/testing support. For example, when major changes or new features are introduced to the codebase, security-related changes are made, and substantial user experience improvements are required. The QA engineer can advise whether that needs to be considered in the current e2e suite, they can advise on edge cases or other scenarios to consider for testing. + +6. Follow up on your PR to keep momentum; the review should happen in 24h business days. If you haven't received feedback from the reviewers after that time, check if they are available. Otherwise, it's okay to request a review from another person. +7. Once the PR has been approved by a QA engineer and a developer, wait for the GitHub Actions to succeed and ensure there are no conflicts with the main branch. +8. Double-check the [commit message format]({{< ref "#commit-message-format" >}}) is correct. Make sure to recognize collaboration in the commit description: `Co-authored-by: `. +9. Merge your work by selecting `Squash and merge`. This will compress all the commits into one, keeping the repository's commit history clean. + - If a backport is required, cherry-pick the merged commit back to the release branches it is required in. ### Done -Issues in this column are complete, all code has been merged into the main branch and/or release branches, and are ready for release. +Issues in this column are complete, all code has been merged into the main branch and/or release branches, and are ready for release. An issue is considered complete when: + + - The documentation is finalized. + - It includes merged updates in CHT Docs, updated README files, and necessary code comments. + - This is especially important as the CHT community can always access up-to-date documentation. + - Unit tests, e2e tests and/or integration tests are written. + - The static checks like linting and Sonar should pass successfully as part of the quality process, and any issues should be fixed. + - The reviewers have approved the Pull Request. + - All code has been merged into the main branch and/or release branches and are ready for release. + - The issue is added to the appropriate release milestone, which is the earliest semver version the change will be released in. This ensures it will be included in the release notes. + - Lastly, the issue status is updated to `Done`.