Merging - Revision history

Rspencer: /* Merge process */

2019-01-18T08:38:30Z

Merge process

Hbecker: /* Code review & approval */ Update code review link

2017-03-31T22:26:13Z

Code review & approval: Update code review link

Hbecker: Add code review section

2017-03-23T23:33:48Z

Add code review section

Hbecker: Copied from internal wiki docs

2017-03-23T22:46:20Z

Copied from internal wiki docs

New page

[[Main Page]] > [[Development]] > Merging & Branches

== Archivematica branching guidelines ==

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: <code>dev/issue-####-short-description</code>
** <code>dev/</code> marks it as being a development branch, not for QA integration or a stable release
** <code>issue-####</code> 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 branches
** <code>short-description</code> is a couple word description of the branch, to make it easier to remember what the topic of the branch is.
** Example: <code>dev/issue-8161-dashboard-i18n</code> or <code>dev/issue-8693-dataverse</code>
* 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.

== Archivematica 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>master</code>. The target branch is usually master, qa/0.x or qa/1.x.

The instructions are formatted: <code>(current branch) $ <command to run></code>

# Check your remote is set to be gitolite
#* <code>$ git remote -v</code> should show <code>origin git@git.artefactual.com:<repository-name>.git</code>
#* Not all repositories have gitolite as the canonical repo - ask if you're unsure
# Ensure your copy of the target branch is up to date
#* <code>$ git checkout master</code>
#* <code>(master) $ git pull --rebase </code>
# Ensure your development branch is based off the latest target branch
#* <code>$ git checkout dev/test</code>
#* <code>(dev/test) $ git rebase master</code>
# Check that your changes still work after rebasing
# Check tests pass (see https://wiki.archivematica.org/Getting_started#Tests for more details)
#* <code>(dev/test) $ py.test</code>
# 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!'''
#* <code>(dev/test) $ git push -f</code>
# Fast-forward merge
#* <code>$ git checkout master</code>
#* <code>(master) $ git merge dev/test</code>
# Push merge!
#* <code>(master) $ git push</code>
# Delete dev branch
#* Locally: <code>(master) $ git branch -d dev/test</code>
#* Remote: <code>(master) $ git push origin --delete dev/test</code>
# Update the related ticket, if needed
# You're done!

== Proposed changes ==

=== Forks ===

* '''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 ===

* '''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

[[Category:Process documentation]]

@@ 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.
-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/pull/547 also available].  (TODO update after PR is merged).  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.
+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.

@@ Line 24: / Line 24: @@
 == 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>master</code>.  The target branch is usually master, qa/0.x or qa/1.x.
+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>
-# Check your remote is set to be gitolite
+# Check your remote is set correctly
-#* <code>$ git remote -v</code> should show <code>origin  git@git.artefactual.com:<repository-name>.git</code>
+#* <code>$ git remote -v</code> should show <code>origin https://github.com/<organisation>/<repository-name></code>
-#* Not all repositories have gitolite as the canonical repo - ask if you're unsure
+#* 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 master</code>
+#* <code>$ git checkout qa/1.x</code>
 #* <code>(master) $ git pull --rebase </code>
 # Ensure your development branch is based off the latest target branch
 #* <code>$ git checkout dev/test</code>
-#* <code>(dev/test) $ git rebase master</code>
+#* <code>(dev/test) $ git rebase qa/1.x</code>
 # Check that your changes still work after rebasing
-# Check tests pass (see https://wiki.archivematica.org/Getting_started#Tests for more details)
+# Check tests pass:
-#* <code>(dev/test) $ py.test</code>
+#* In the docker-compose environment use the <code>MakeFile</code> tests, e.g. <code>make test-mcp-client</code>
 # 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!'''
 #* <code>(dev/test) $ git push -f</code>
 # Fast-forward merge
-#* <code>$ git checkout master</code>
+#* <code>$ git checkout qa/1.x</code>
 #* <code>(master) $ git merge dev/test</code>
 # Push merge!

@@ Line 1: / Line 1: @@
 [[Main Page]] > [[Development]] > Merging & Branches
-== Archivematica branching guidelines ==
+== Branching guidelines ==
 Guidelines on when to create branches for new development in Archivematica projects.  Short answer: always create branches!
@@ Line 14: / Line 14: @@
 * 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.
-== Archivematica merge process ==
+== Code review & approval ==
 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>master</code>.  The target branch is usually master, qa/0.x or qa/1.x.