Difference between revisions of "Merging"
(Add code review section) |
|||
(One intermediate revision by one other user not shown) | |||
Line 18: | Line 18: | ||
All code changes merged into a stable or qa branch must go through code review. We use Github's pull request feature for code review. Every change being submitted to an Artefactual project should be submitted as a pull request to the appropriate repository. | All code changes merged into a stable or qa branch must go through code review. We use Github's pull request feature for code review. Every change being submitted to an Artefactual project should be submitted as a pull request to the appropriate repository. | ||
− | Another core contributor (usually an Artefactual employee) will review the code for content, style and suitability in the project. A more extensive guideline to code review is [https://github.com/artefactual/archivematica/ | + | Another core contributor (usually an Artefactual employee) will review the code for content, style and suitability in the project. A more extensive guideline to code review is [https://github.com/artefactual/archivematica/blob/qa/1.x/code_review.md also available]. After the reviewer is satisfied with the code, they will approve it on github. If at least one reviewer approves, and there are no disapprovals, the branch can be merged. |
In general, the original contributor will [[#Merge process | merge the branch]]. For external contributions, the reviewer can merge or cherry-pick the commits. This will leave the Author correct in git, but update the committer. | In general, the original contributor will [[#Merge process | merge the branch]]. For external contributions, the reviewer can merge or cherry-pick the commits. This will leave the Author correct in git, but update the committer. | ||
Line 24: | Line 24: | ||
== Merge process == | == Merge process == | ||
− | We use a rebase-based workflow, where branches are brought up to date with the latest stable by rebasing them. (This is compared to a merge-based workflow, where branches are merged, and conflicts resolved in the merge commit). This shows the process for merging development branch <code>dev/test</code> into target branch <code> | + | We use a rebase-based workflow, where branches are brought up to date with the latest stable by rebasing them. (This is compared to a merge-based workflow, where branches are merged, and conflicts resolved in the merge commit). This shows the process for merging development branch <code>dev/test</code> into target branch <code>qa/1.x</code>. The target branch is usually qa/1.x. |
The instructions are formatted: <code>(current branch) $ <command to run></code> | The instructions are formatted: <code>(current branch) $ <command to run></code> | ||
− | # Check your remote is set | + | # Check your remote is set correctly |
− | #* <code>$ git remote -v</code> should show <code>origin | + | #* <code>$ git remote -v</code> should show <code>origin https://github.com/<organisation>/<repository-name></code> |
− | #* | + | #* Archivematica uses GitHub now as its primary host with a mirror in GitLab so it would be very rare that your <code>origin</code> would be something different. |
− | # Ensure your copy of the target branch is up to date | + | # Ensure your copy of the target branch is up to date, in most cases this will be <code>qa/1.x</code> |
− | #* <code>$ git checkout | + | #* <code>$ git checkout qa/1.x</code> |
#* <code>(master) $ git pull --rebase </code> | #* <code>(master) $ git pull --rebase </code> | ||
# Ensure your development branch is based off the latest target branch | # Ensure your development branch is based off the latest target branch | ||
#* <code>$ git checkout dev/test</code> | #* <code>$ git checkout dev/test</code> | ||
− | #* <code>(dev/test) $ git rebase | + | #* <code>(dev/test) $ git rebase qa/1.x</code> |
# Check that your changes still work after rebasing | # Check that your changes still work after rebasing | ||
− | # Check tests pass | + | # Check tests pass: |
− | #* <code> | + | #* In the docker-compose environment use the <code>MakeFile</code> tests, e.g. <code>make test-mcp-client</code> |
+ | #* Push your rebased development branch back to GitHub and ensure the Travis CI checks pass before merging (see below). | ||
# Push your newly-rebased branch to remote. Because this is a rebase, which changes history, you'll need --force. The rebased dev branch has to be pushed before the master branch is or github gets confused. | # Push your newly-rebased branch to remote. Because this is a rebase, which changes history, you'll need --force. The rebased dev branch has to be pushed before the master branch is or github gets confused. | ||
#* '''Never force push on a stable/, qa/ or master branch!''' | #* '''Never force push on a stable/, qa/ or master branch!''' | ||
#* <code>(dev/test) $ git push -f</code> | #* <code>(dev/test) $ git push -f</code> | ||
# Fast-forward merge | # Fast-forward merge | ||
− | #* <code>$ git checkout | + | #* <code>$ git checkout qa/1.x</code> |
#* <code>(master) $ git merge dev/test</code> | #* <code>(master) $ git merge dev/test</code> | ||
# Push merge! | # Push merge! |
Latest revision as of 02:38, 18 January 2019
Main Page > Development > Merging & Branches
Branching guidelines[edit]
Guidelines on when to create branches for new development in Archivematica projects. Short answer: always create branches!
- All development work happens in branches - nothing is committed directly to master, qa/ or stable/ branches
- Development branches should follow the naming format:
dev/issue-####-short-description
dev/
marks it as being a development branch, not for QA integration or a stable releaseissue-####
is the Redmine ticket that the work is mostly related to. If the branch expands in scope to include multiple tickets later, that's fine, and don't worry about renaming branchesshort-description
is a couple word description of the branch, to make it easier to remember what the topic of the branch is.- Example:
dev/issue-8161-dashboard-i18n
ordev/issue-8693-dataverse
- Development branches should generally be create from the branch they will be merged into. In most cases, this is master, qa/1.x (Archivematica) or qa/0.x (Storage Service). If something is a patch for an old release, it may be more appropriate to branch from the old stable branch (e.g. stable/1.5.x)
- All branches should undergo code review before being merged into the target branch. If something was developed for an old release, it should be cherry-picked to the current development branch, and a comment with the commit hash should be left in the pull request for tracking.
Code review & approval[edit]
All code changes merged into a stable or qa branch must go through code review. We use Github's pull request feature for code review. Every change being submitted to an Artefactual project should be submitted as a pull request to the appropriate repository.
Another core contributor (usually an Artefactual employee) will review the code for content, style and suitability in the project. A more extensive guideline to code review is also available. After the reviewer is satisfied with the code, they will approve it on github. If at least one reviewer approves, and there are no disapprovals, the branch can be merged.
In general, the original contributor will merge the branch. For external contributions, the reviewer can merge or cherry-pick the commits. This will leave the Author correct in git, but update the committer.
Merge process[edit]
We use a rebase-based workflow, where branches are brought up to date with the latest stable by rebasing them. (This is compared to a merge-based workflow, where branches are merged, and conflicts resolved in the merge commit). This shows the process for merging development branch dev/test
into target branch qa/1.x
. The target branch is usually qa/1.x.
The instructions are formatted: (current branch) $ <command to run>
- Check your remote is set correctly
$ git remote -v
should showorigin https://github.com/<organisation>/<repository-name>
- Archivematica uses GitHub now as its primary host with a mirror in GitLab so it would be very rare that your
origin
would be something different.
- Ensure your copy of the target branch is up to date, in most cases this will be
qa/1.x
$ git checkout qa/1.x
(master) $ git pull --rebase
- Ensure your development branch is based off the latest target branch
$ git checkout dev/test
(dev/test) $ git rebase qa/1.x
- Check that your changes still work after rebasing
- Check tests pass:
- In the docker-compose environment use the
MakeFile
tests, e.g.make test-mcp-client
- Push your rebased development branch back to GitHub and ensure the Travis CI checks pass before merging (see below).
- In the docker-compose environment use the
- Push your newly-rebased branch to remote. Because this is a rebase, which changes history, you'll need --force. The rebased dev branch has to be pushed before the master branch is or github gets confused.
- Never force push on a stable/, qa/ or master branch!
(dev/test) $ git push -f
- Fast-forward merge
$ git checkout qa/1.x
(master) $ git merge dev/test
- Push merge!
(master) $ git push
- Delete dev branch
- Locally:
(master) $ git branch -d dev/test
- Remote:
(master) $ git push origin --delete dev/test
- Locally:
- Update the related ticket, if needed
- You're done!
Proposed changes[edit]
Forks[edit]
- Current status: All development branches are created in the artefactual (or artefactual-labs) owned repository, and core contributors (usually Artefactual employees) commit directly to the official repository
- Proposed change: Development happens in contributor's forks, and the official repo contains only stable & QA branches
- Pros:
- Fewer branches in the official repository, so there is less clutter and noise for people watching activity
- Handles more contributors, since there won't be dozens or hundreds of branches of uncertain development status
- Contribution process is the same for external and internal contributors
- Contributors can handle their repository branches however they want - naming conventions, deleting old branches or not, etc
- Cons:
- Potential confusion about which repository is the correct official one
- Potential to push changes to the wrong repo (e.g. push a development branch to the official repo instead of the contributor's fork)
- Developers have to keep 2 repositories up to date: upstream and their fork
- Harder for analysts to check out development code, since they would have to set their remote to the contributor's fork
Hybrid merge/rebase model[edit]
- Current status: All branches are rebased onto their target branch before being (fast-forward) merged
- Proposed change: Small or minor branches are rebased, larger feature branches are true merged
- Pros:
- Commit history clearly shows 'feature', and which commits came in together as a unit
- Rebasing small branches preserves clean history for minor changes
- Better reflects nature of development - shows minor changes as happening inline, and major changes as happening in parallel
- Merge commits are clear places to look for integration issues
- Cons:
- Confusion about when to merged vs rebase
- Developers have to learn two workflows