|
| 1 | +# Contribution workflow and the review process |
| 2 | + |
| 3 | +This document outlines the best practice, using git and CI, which **must** be followed |
| 4 | +for all contributions to `mlip-testing`. Also contained are instructions and tips for |
| 5 | +managing your fork of the project, which will help keep merges clean and avoid many |
| 6 | +headaches. |
| 7 | + |
| 8 | +Please read our |
| 9 | +[developer documentation](https://ddmms.github.io/mlip-testing/developer_guide/index.html) |
| 10 | +for more technical details relating to contributions. |
| 11 | + |
| 12 | +## Golden rules |
| 13 | + |
| 14 | +In brief the rules for contribution are as follows: |
| 15 | + |
| 16 | +- Follow the branch, fix, merge model, from your own fork or fork/branch model |
| 17 | +- Use the appropriate issue and pull request templates |
| 18 | +- An issue must be created for every piece of work (new benchmark, bug, feature, *etc.*) |
| 19 | +- Pull/merge requests will not be accepted without a review |
| 20 | +- New features must have a test |
| 21 | +- All tests must pass, no regressions may be merged |
| 22 | + |
| 23 | +## Contributing benchmarks/models |
| 24 | + |
| 25 | +Please consult our online documentation on |
| 26 | +[adding benchmarks](https://ddmms.github.io/mlip-testing/developer_guide/add_benchmarks.html) |
| 27 | +and |
| 28 | +[adding models](https://ddmms.github.io/mlip-testing/developer_guide/add_models.html) |
| 29 | +for a detailed description of the process involved. |
| 30 | + |
| 31 | +## Issues |
| 32 | + |
| 33 | +### Using issues |
| 34 | + |
| 35 | +- Open an issue for each piece of work done, using the appropriate issue template |
| 36 | +- Open issues for design discussions. The answers may be important for newer members |
| 37 | +- When adding features, use comments to provide succinct reports on progress |
| 38 | + |
| 39 | +### Labels |
| 40 | + |
| 41 | +Labels may be assigned to issues to help classify them. Examples include: |
| 42 | + |
| 43 | +- `new benchmark`: Proposing or discussing a new benchmark |
| 44 | +- `bug`: something is not working as expected. We typically aim to fix these ASAP |
| 45 | +- `Enhancement`: adding or improving on features |
| 46 | +- `testing`: adding or enhancing unit testing or CI |
| 47 | +- `documentation`: adding or enhancing documentation |
| 48 | +- `question`: Something is unclear |
| 49 | + |
| 50 | +## Pull requests |
| 51 | + |
| 52 | +### Creating pull requests |
| 53 | + |
| 54 | +- Select the most appropriate template when creating your pull request |
| 55 | +- Each pull request should aim to resolve an open issue |
| 56 | +- Relevant questions and discussions should be noted in the pull request, even if |
| 57 | + discussed elsewhere, as these may be useful for reference or other developers |
| 58 | +- Creating draft pull requests as early as possible is encouraged, to track progress, |
| 59 | + and so early feedback can be given |
| 60 | + |
| 61 | +### Labels |
| 62 | + |
| 63 | +As with issues, labels may be assigned to issues to help classify them. |
| 64 | + |
| 65 | +## Review |
| 66 | + |
| 67 | +All merge requests will be reviewed to ensure the integrity of the code. |
| 68 | + |
| 69 | +The reviewer(s) have the following responsibilities: |
| 70 | + |
| 71 | +- Ensuring all contribution rules have been followed |
| 72 | +- Ensuring the [coding style](./coding_style.md) is adhered to |
| 73 | +- Only accepting a merge if all tests have passed |
| 74 | +- Using the comments system to request changes for the submitter to make |
| 75 | + |
| 76 | +### Enforcing style |
| 77 | + |
| 78 | +GitHub Actions will automatically run a pre-commit and enforce the coding style. To |
| 79 | +reduce the number of CI failures, please run the pre-commit locally before you push to |
| 80 | +your repository. |
| 81 | + |
| 82 | +```sh |
| 83 | + pre-commit run --all-files |
| 84 | +``` |
| 85 | + |
| 86 | +## Using git for development |
| 87 | + |
| 88 | +The GitHub instance hosts an *upstream* repository, which we will refer to as |
| 89 | +*upstream*. Contributors will work on their own personal copies of the repository by |
| 90 | +creating *forks*. This allows us to keep *upstream* clean, while users may work on |
| 91 | +their own *fork*, creating commits and changing the code as they see fit. Only in |
| 92 | +exceptional circumstances are branches allowed in *upstream*. |
| 93 | + |
| 94 | +The *upstream* repository may be cloned as follows, |
| 95 | + |
| 96 | +``` sh |
| 97 | +git clone [email protected]:ddmms/mlip-testing.git |
| 98 | +``` |
| 99 | + |
| 100 | +A *fork* is created using the web UI. It may then be cloned for a user called |
| 101 | +'username' as follows: |
| 102 | + |
| 103 | +``` sh |
| 104 | +git clone [email protected]:username/mlip-testing.git mlip-testing-username |
| 105 | +``` |
| 106 | + |
| 107 | +or added as an alternative origin in the *upstream* cloned repository: |
| 108 | + |
| 109 | +``` sh |
| 110 | +git remote add username [email protected]:username/mlip-testing.git |
| 111 | +``` |
| 112 | + |
| 113 | +### Branch, fix, merge model: |
| 114 | + |
| 115 | +All work should follow the workflow of branch, fix, merge. Let us assume you have an |
| 116 | +issue with your code which needs to be fixed. |
| 117 | + |
| 118 | +#### Step 1: Branch from your fork |
| 119 | + |
| 120 | +Create a new branch for the issue on the dashboard of your fork, we will assume the |
| 121 | +branch is called 'fix-xyz'. Clone the branch, |
| 122 | + |
| 123 | +``` sh |
| 124 | +$ git clone -b fix-xyz --single-branch [email protected]:username/mlip-testing.git mlip-testing-fix-xyz |
| 125 | +``` |
| 126 | + |
| 127 | +Alternatively you can create the branch in the CLI using |
| 128 | + |
| 129 | +``` sh |
| 130 | +# clone the repository, if you already have a local repository this is not nessecary |
| 131 | +$ git clone [email protected]:username/mlip-testing.git mlip-testing-fix-xyz |
| 132 | +$ pushd mlip-testing-fix-xyz |
| 133 | +# create and checkout a new branch (this is equivalent to git branch followed by git checkout) |
| 134 | +$ git checkout -b fix-xyz |
| 135 | +# create a remote tracking branch for you to push your changes to |
| 136 | +$ git push -u origin fix-xyz |
| 137 | +``` |
| 138 | + |
| 139 | +#### Step 2: Fix the issue and commit your changes |
| 140 | + |
| 141 | +Fix whatever is wrong. Use `git status` to see which files you have |
| 142 | +changed and prepare a commit. |
| 143 | + |
| 144 | +``` sh |
| 145 | +# stage changes |
| 146 | +$ git add <filename|folder> # to add the new things |
| 147 | +# commit the changes with a clear and brief message |
| 148 | +$ git commit -m "<commit message>" |
| 149 | +# push the commit to origin |
| 150 | +$ git push |
| 151 | +``` |
| 152 | + |
| 153 | +#### Step 3a: Merge your branch into upstream |
| 154 | + |
| 155 | +Follow the provided link after the push and continue to the web interface. Add any |
| 156 | +relevant labels or milestones and assign a reviewer. Compare the code and if you are |
| 157 | +happy click to submit your pull request. |
| 158 | + |
| 159 | +After the pull request has been submitted, tests will be run and your reviewer will be |
| 160 | +notified. |
| 161 | + |
| 162 | +#### Step 3b: Finalising the merge |
| 163 | + |
| 164 | +If all is OK with the commit your reviewer may set the request to be merged once all |
| 165 | +tests pass. Otherwise, the reviewer may open discussions using the GitHub comment |
| 166 | +system to point out issues that may need to be addressed before the commit can be |
| 167 | +merged. |
| 168 | + |
| 169 | +If changes need to be made you may make more commits onto your branch. When you push |
| 170 | +your branch to your *fork* the pull request will be automatically updated to use the |
| 171 | +latest commit. Reply to the discussions to indicate when and how they have been |
| 172 | +addressed. |
| 173 | + |
| 174 | +If your branch has become out of sync with *upstream* then conflicts may arise. |
| 175 | +Sometimes these cannot be automatically resolved and you will need to resolve them by |
| 176 | +hand. GitHub provides instructions for this, or you can follow this routine, |
| 177 | + |
| 178 | +``` sh |
| 179 | +# add upstream as a remote if you have not already |
| 180 | +$ git remote add upstream [email protected]:ddmms/mlip-testing.git |
| 181 | +# get the changes to upstream since you started working on your issue |
| 182 | +$ git fetch upstream |
| 183 | +# merge these changes into your branch (assuming you want to merge into the main branch on upstream) |
| 184 | +$ git merge upstream/main |
| 185 | +# resolve any conflicts |
| 186 | +# push to your fork |
| 187 | +$ git push |
| 188 | +``` |
| 189 | + |
| 190 | +Alternatively you may use rebase which will replay the changes you made in your branch |
| 191 | +on top of *upstream/main*. However, be sure you understand the differences between |
| 192 | +merge and rebase. |
| 193 | + |
| 194 | +``` sh |
| 195 | +# add upstream as a remote if you have not already |
| 196 | +$ git remote add upstream [email protected]:ddmms/mlip-testing.git |
| 197 | +# get the changes to upstream since you started working on your issue |
| 198 | +$ git fetch upstream |
| 199 | +# merge these changes into your branch (assuming you want to merge into the main branch on upstream) |
| 200 | +$ git rebase -i upstream/main |
| 201 | +# resolve any conflicts |
| 202 | +# push to your fork |
| 203 | +$ git push |
| 204 | +``` |
| 205 | + |
| 206 | + |
| 207 | +### Advanced git |
| 208 | + |
| 209 | +#### Keeping your fork in sync with project |
| 210 | + |
| 211 | +By adding two remotes, one for *upstream* and one for your *fork* it is possible to |
| 212 | +keep your *fork* in sync with *upstream*. This will greatly simplify merge requests. |
| 213 | +GitHub also offers a sync functionality in their web UI that achieves the same. |
| 214 | + |
| 215 | +``` sh |
| 216 | +# clone your fork |
| 217 | +$ git clone [email protected]:username/mlip-testing.git mlip-testing-username |
| 218 | +pushd mlip-testing-username |
| 219 | +# add a remote for upstream |
| 220 | +$ git remote add upstream [email protected]:ddmms/mlip-testing.git |
| 221 | +``` |
| 222 | + |
| 223 | +These commands need to be done only once. `git remote -v` shall show you |
| 224 | +the origin and project fetch and push links |
| 225 | + |
| 226 | +``` sh |
| 227 | +$ git remote -v |
| 228 | +origin [email protected]:username/mlip-testing.git (fetch) |
| 229 | +origin [email protected]:username/mlip-testing.git (push) |
| 230 | +upstream [email protected]:ddmms/mlip-testing.git (fetch) |
| 231 | +upstream [email protected]:ddmms/mlip-testing.git (push) |
| 232 | +``` |
| 233 | + |
| 234 | +When you need to sync your *fork* with *upstream*, do the following, |
| 235 | + |
| 236 | +``` sh |
| 237 | +# get the latest commits from upstream |
| 238 | +$ git fetch upstream |
| 239 | +# ensure you are in the main branch of your fork |
| 240 | +$ git checkout main |
| 241 | +# merge your main branch into the main branch of upstream |
| 242 | +$ git merge upstream/main |
| 243 | +# push these changes back to the remote of your fork (origin) |
| 244 | +$ git push |
| 245 | +``` |
| 246 | + |
| 247 | +Of course, one can use a similar process to merge any other branch or available |
| 248 | +projects. |
| 249 | + |
| 250 | +#### Rebasing commits |
| 251 | + |
| 252 | +When working on an issue you may use multiple commits. When you are ready to create a |
| 253 | +merge request, you should squash your changes into one commit in order to keep |
| 254 | +*upstream* clean. This is most easily achieved with an interactive rebase. |
| 255 | + |
| 256 | +Assuming you have made five commits, |
| 257 | + |
| 258 | +``` sh |
| 259 | +# rebase your branch five commits before HEAD i.e. where your branch originally diverged |
| 260 | +$ git rebase -i HEAD~5 |
| 261 | +# follow the instructions. 'pick' the first commit then 'sqaush' or 'fixup' the rest. |
| 262 | +# You should now be left with a single commit containing all your changes |
| 263 | +# Push your commmit to the remote, use --force-with-lease if you have already pushed this branch to |
| 264 | +# 'rewrite history' |
| 265 | +$ git push origin branchname --force-with-lease |
| 266 | +``` |
| 267 | + |
| 268 | +Using force is a powerful and dangerous option, use it only if you know 150% nobody |
| 269 | +else touched that branch. |
| 270 | + |
| 271 | +#### Cleaning stale branches |
| 272 | + |
| 273 | +Deleting branches from the web interface will get rid of the remotes and not of your |
| 274 | +local copies. The local branches left behind are called stale branches. To get rid of |
| 275 | +them: |
| 276 | + |
| 277 | +``` sh |
| 278 | +$ git remote prune origin |
| 279 | +``` |
| 280 | + |
| 281 | +To delete a local branch: |
| 282 | + |
| 283 | +``` sh |
| 284 | +$ git branch -d localBranch |
| 285 | +``` |
| 286 | + |
| 287 | +If unmerged commits exists, but you still want to delete the branch, use: |
| 288 | + |
| 289 | +``` sh |
| 290 | +$ git branch -D localBranch |
| 291 | +``` |
| 292 | + |
| 293 | +To delete a remote branch on the remote *origin* use: |
| 294 | + |
| 295 | +``` sh |
| 296 | +$ git push -d origin remoteBranch |
| 297 | +``` |
| 298 | + |
| 299 | +## Code Coverage |
| 300 | + |
| 301 | +Ensure that any code you add does not reduce the code coverage in a meaningful way. |
| 302 | +Reviewers may insist on new test to be added. Please cooperate. |
0 commit comments