Introduce Git / GitHub review guidelines #3
Conversation
Introduce Git / GitHub review guidelines as Markdown document. This is to be added / referenced as review guidelines to other repositories. Signed-off-by: Razvan Deaconescu <[email protected]>
razvand
force-pushed
the
github/review
branch
from
c2bf7c3 to
50b9b2c
Compare
| Use the command to browse through the metadata of commits: | ||
|
|
||
| ```console | ||
| git log |
There was a problem hiding this comment.
| git log | |
| git log --format=full |
Otherwise, you will not see the committer name and email.
|
|
||
| For the second item, you will configure your local repository to point to the branch used to create the pull request. | ||
| For this, do the following: | ||
|
|
There was a problem hiding this comment.
I would mention gh here, since it saves a lot of time with the setup.
Not give instructions, just mention that everything in this step can be done via GitHub cli, and link to docs somewhere else (e.g. https://eszter.space/gh-cli/)
There was a problem hiding this comment.
Additionally, mention pressing . on the GitHub interface of the PR. It opens a cloud Code UI that has the branch of the PR already checked in so you don't even need to have your own clone locally.
| When reviewing documentation contributions, make sure you run the documented actions (commands or otherwise) to ensure they work OK. | ||
|
|
||
| Testing is generally particular to the repository. | ||
| Do your best in ensuring that contributions work as expectedly and they don't introduce additional bugs. |
There was a problem hiding this comment.
I would insist on testing the actual features the contribution adds, not just testing that nothing breaks. I feel like this is often missed both by the reviewers and by the automated tests.
Somthing like:
If the contributions add a new feature, make sure to test it properly, do not focus soley on regression testing.
Ask the author to provide instructions on how to test the new features, or, even better, to add test cases to the pull request.
There was a problem hiding this comment.
it depends on the project, most of the projects have QA team to test that feature. Otherwise it's pretty time-consuming for you as a developer to test everything. I think you should mention both situations here.
| The second part means that you have access to the pull request (in case of a private) repository. | ||
|
|
||
| In order to be formally assigned as a reviewer to a pull request in the GitHub interface, you have to have the [`Triage` role the repository](https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization). | ||
| This role is assigned to you by a admin member of the repository. |
There was a problem hiding this comment.
| This role is assigned to you by a admin member of the repository. | |
| This role is assigned to you by an admin member of the repository. |
| This role is not a requirement, you can do a review without being assigned as a reviewer. | ||
|
|
||
| 1. Have a local clone of the repository (or a clone of your fork of the repository). | ||
| So you either did, at point, `git clone <URL>` using the `<URL>` of the main repository, or using the `<URL`> of your [fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks) of that repository. |
There was a problem hiding this comment.
| So you either did, at point, `git clone <URL>` using the `<URL>` of the main repository, or using the `<URL`> of your [fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks) of that repository. | |
| So you either did, at point, `git clone <URL>` using the `<URL>` of the main repository, or using the `<URL>` of your [fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks) of that repository. |
| 1. Have a local clone of the repository (or a clone of your fork of the repository). | ||
| So you either did, at point, `git clone <URL>` using the `<URL>` of the main repository, or using the `<URL`> of your [fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks) of that repository. | ||
|
|
||
| For the first item, you browser should have opened the GitHub pull request. |
There was a problem hiding this comment.
| For the first item, you browser should have opened the GitHub pull request. | |
| For the first item, your browser should have opened the GitHub pull request. |
| E.g. `https://github.com/cluosh/unikraft`, `https://github.com/michpappas/unikraft`. | ||
|
|
||
| 1. **Note**: This step is required if this is the first time you add a given remote. | ||
| If you have added a remote for the username before, you ca skip this step. |
There was a problem hiding this comment.
| If you have added a remote for the username before, you ca skip this step. | |
| If you have added a remote for the username before, you can skip this step. |
| - hard to read content | ||
| - typos | ||
| - violation of general coding / writing conventions | ||
| - lines to large |
There was a problem hiding this comment.
| - lines to large | |
| - lines too large |
| - violation of general coding / writing conventions | ||
| - lines to large | ||
| - missing newlines at the end of the files | ||
| - training white spaces |
There was a problem hiding this comment.
| - training white spaces | |
| - trailing white spaces |
|
|
||
| Reviewing implies 5 actions: | ||
|
|
||
| 1. Reviewing the commit messages and metadata (signature, authorship) |
There was a problem hiding this comment.
| 1. Reviewing the commit messages and metadata (signature, authorship) | |
| 1. Reviewing the commit messages and metadata (signature, authorship, CLA/DCO) |
There was a problem hiding this comment.
All of these are requirements that vary from repo to repo, not all are mandatory and absence of some means the review process is simplified.
|
|
||
| 1. Reviewing the commit messages and metadata (signature, authorship) | ||
|
|
||
| 1. Reviewing the content of the contribution: good design, implementation value, readability, compliance |
There was a problem hiding this comment.
| 1. Reviewing the content of the contribution: good design, implementation value, readability, compliance | |
| 1. Reviewing the content of the contribution: good design, implementation value, readability, compliance, presence of adequate tests |
| 1. Be logged in on GitHub and have reviewer access to the pull request. | ||
| The second part means that you have access to the pull request (in case of a private) repository. | ||
|
|
||
| In order to be formally assigned as a reviewer to a pull request in the GitHub interface, you have to have the [`Triage` role the repository](https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization). |
There was a problem hiding this comment.
Triage means you can be assigned as reviewer, but in order for the review to have a green checkmark you need write access to the repo.
|
|
||
| In order to be formally assigned as a reviewer to a pull request in the GitHub interface, you have to have the [`Triage` role the repository](https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization). | ||
| This role is assigned to you by a admin member of the repository. | ||
| This role is not a requirement, you can do a review without being assigned as a reviewer. |
There was a problem hiding this comment.
Repos can also be configured to only allow reviews from contributors (people with write access) only. In that case, the review interface will have the "approve"/"request changes" radio buttons disabled.
|
|
||
| For the second item, you will configure your local repository to point to the branch used to create the pull request. | ||
| For this, do the following: | ||
|
|
There was a problem hiding this comment.
Additionally, mention pressing . on the GitHub interface of the PR. It opens a cloud Code UI that has the branch of the PR already checked in so you don't even need to have your own clone locally.
| The title line typically has a prefix that points to the type of contribution and/or the location in the repository. | ||
| This is generally stated as part of the repository contributor conventions. |
There was a problem hiding this comment.
Other repos use [feat]/[nfc]/[bug]/[fix] prefixes instead of the location. It is better to consult the repository's contribution guidelines to make sure.
There was a problem hiding this comment.
https://www.conventionalcommits.org/en/v1.0.0/ is pretty standard I think, tho as @mihaimaruseac depends on the project.
| 1. If the commit conventions use or imply using a [`Signed-off-by` line](https://stackoverflow.com/a/14044024/4804196), make sure that line also has the author properly stated. | ||
| Also make sure the exact same name are used for both the author and the `Signed-off-by` line. |
There was a problem hiding this comment.
This can be automated by a DCO automated check
| In the GitHub interactive interface in the `Files changed` tab you can request changes either on a single line or on multiple lines, where the `+` icon appears when hovering. | ||
| You can request changes as feedback text or as concrete content update suggestions, as shown in the [GitHub review guidelines](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request) and add your request for changes. | ||
|
|
||
| Locally you can get a unified view of the changes inside files. |
There was a problem hiding this comment.
Maybe mention the . trick here too?
| Finally, you want to be sure that the content is well aligned with the commits it is part of. | ||
| Each commit should do one thing and do one thing well, and the content and commit should be aligned. |
There was a problem hiding this comment.
This assumes that the repo reviews each commit individually, but there are other repos that review at PR level, squashing all commits upon merging the PR.
| When checking the commit be sure that: | ||
|
|
||
| - The commit message and the commit contents correspond. | ||
| The commit message should not state anything more or less than the contents. |
There was a problem hiding this comment.
What about "The commit messages describes the contents entirely and nothing more than the contents"? The "more or less" part makes this confusing.
|
|
||
| 1. Reviewing the content of the contribution: good design, implementation value, readability, compliance | ||
|
|
||
| 1. Testing changes, making sure the fix the problem or they add the proposed feature, making sure they don't break anything |
There was a problem hiding this comment.
| 1. Testing changes, making sure the fix the problem or they add the proposed feature, making sure they don't break anything | |
| 1. Testing changes, making sure they fix the problem or they add the proposed feature without breaking anything |
|
|
||
| The name of the local branch is something you choose. | ||
| Generally, it is advised to keep the same name as `<branch>`. | ||
| However, it may be the case that the contributor uses a value for `<branch>` that conflicts with an existing local one (such as `main` or `master` or `testing`). |
There was a problem hiding this comment.
| However, it may be the case that the contributor uses a value for `<branch>` that conflicts with an existing local one (such as `main` or `master` or `testing`). | |
| However, the contributor may use a value for `<branch>` that conflicts with an existing local one (such as `main` or `master` or `testing`). |
| git checkout -b michpappas/feature/libukmemtag michpappas/michpappas/feature/libukmemtag | ||
| ``` | ||
|
|
||
| In case of the first command, we used `<username>-<branch>` for the local branch. |
There was a problem hiding this comment.
| In case of the first command, we used `<username>-<branch>` for the local branch. | |
| In the case of the first command, we used `<username>-<branch>` for the local branch. |
| Ideally, the repository has CI/CD pipelines configured that automate testing of common items. | ||
| This generally relies on the repository itself featuring tests that can be called automatically. | ||
|
|
||
| Even if this is the case, there may be specific tests that you want do manually. |
There was a problem hiding this comment.
| Even if this is the case, there may be specific tests that you want do manually. | |
| Even if this is the case, there may be specific tests that you want to do manually. |
|
|
||
| In the `Files changed` tab, click on the `Review changes` button and add your review in the pop-up text area. | ||
| This is a step that you will do from the beginning, and keep adding contents to the review message as you go through the review changes. | ||
| Note that if you click outside of the pop-up text are, it will close, but the review message is still there; |
There was a problem hiding this comment.
| Note that if you click outside of the pop-up text are, it will close, but the review message is still there; | |
| Note that if you click outside the pop-up text area, it will close, but the review message is still there; |
| This is useful because reviewing is an ongoing process and you may want to switch between filling the pop-up text area, adding suggestions, or checking the commits in the `Commits` tab. | ||
|
|
||
| Once the text and suggestions of your review are complete, in the pop-up text area check on of the three buttons: `Comment`, `Approve` or `Request changes`. | ||
| Typically, you choose `Approve` in case all is OK with the pull request and it can be merged, from your point of view; |
There was a problem hiding this comment.
I suggest using 'everything is OK' instead of 'all is OK'
| With a new review submitted, the author will address comments and you will iterate. | ||
|
|
||
| For larger projects, it's common to have multiple review rounds. | ||
| This, of course depends, on the contribution complexity. |
There was a problem hiding this comment.
| This, of course depends, on the contribution complexity. | |
| This, of course, depends on the contribution complexity. |
Introduce Git / GitHub review guidelines as Markdown document. This is to be added / referenced as review guidelines to other repositories.