Merging pr's right after each other might fail the build

[epeee]

Problem

Currently, the build fails if two pr's are merged right after each other, e.g. see build on travis

[./gradlew]   To https://github.com/mockito/shipkit.git
[./gradlew]    * [new tag]           v0.9.61 -> v0.9.61
[./gradlew]    ! [rejected]          master -> master (non-fast-forward)
[./gradlew]   error: failed to push some refs to 'https://dummy:[SECRET]@github.com/mockito/shipkit.git'
[./gradlew]   hint: Updates were rejected because the tip of your current branch is behind
[./gradlew] 
[./gradlew] BUILD FAILED
[./gradlew]   hint: its remote counterpart. Integrate the remote changes (e.g.
[./gradlew]   hint: 'git pull ...') before pushing again.
[./gradlew]   hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Details

When 2 PRs are merged in a short time, there is a chance the Travis CI builds will overlap. This will cause one of the builds to fail. Consider the sequence:

1. merge PR 'A', build 'A' starts
2. merge PR 'B' (clean merge, no conflicts), build 'B' starts even though 'A' hasn't finished
3. 'A' build fails on 'git push' because remote branch has new commit (merge commit from PR 'B', the previous step)

Solution

See comments in the ticket for the discussion about available options. The best approach is to stop writing to master branch during release builds. Here's how we can do it:

• stop pushing release notes to a file on master. Instead we will just write to GH releases. In the future we can add support for release notes elsewhere, too (javadoc file, GH pages in separate branch/repo, etc.)
• stop pushing version bumps to master. Instead, we will infer the patch version dynamically from the commits. We will keep the "version.properties" file (with content like "version=2.10.*").

Implementation

There are other Gradle plugins that implement SemVer-style version bumps (most credible: https://github.com/ajoberstar/reckon). The strategies:

• user-provided version (e.g. command line flag)
• infer from the nearest tag (great example: https://github.com/allegro/axion-release-plugin)
• infer from git describe, e.g. 0.1.0-beta.0.1+e06c68a

How about we take a different approach and infer the version from:

• user configuration - e.g. "1.5.*"
• the number of merge commits since the nearest tag (or any commits if there are no merge commits)

Examples:

Latest tag	version.properties	commits since tag	result	description
1.5.10	1.5.*	1	1.5.11	patch version incremented by # of commits
1.5.10	1.5.*	5	1.5.16	as above
1.5.10	1.6.0	5	1.6.0	use version 'as is'
1.5.10	1.6.*	5	1.6.0	first x.y.0 version
1.5.10	2.0.*	5	2.0.0	first z.0.0 version

This should work well for the typical PR merge use case because there is 1 merge commit and the patch version is incremented by 1. When there are direct commits on master there will be gaps in patch versions but it's not an issue because the most important is to have a unique version.

This approach will not work when there is commit rewriting on master but this never happens (and I think GH prevents it anyway).

Architecuture

I suggest a separate plugin, in a separate repo to make the experimentation easier. This also gives us time to decide how to integrate with main Shipkit plugin. New plugin repo / plugin ID: "org.shipkit.smart-versioning"

[mockitoguy]

Nice ticket! Thank you for describing the scenario and linking the Travis CI failure.

[cgtz]

Hi @mockitoguy, any updates to this issue? It affects our project every once in a while: https://travis-ci.org/linkedin/ambry/builds/621384294

Another issue is that, after this happens, the build does not delete the tag it created in previous steps, leading to all builds after this one failing until a developer manually intervenes and deletes the git tag: https://travis-ci.org/linkedin/ambry/builds/621388169

[performRelease] > Task :gitTag FAILED
[performRelease]   Creating tag:
[performRelease]     git tag -a v0.3.68 -m Created new tag v0.3.68 [ci skip]
[performRelease]   External process [tag] completed.
[performRelease] 7 actionable tasks: 7 executed
[performRelease] [tag] fatal: tag 'v0.3.68' already exists

Do you have any suggestions for either of these issues?

[mockitoguy]

I'll work on this. Thank you guys for reporting!

[mockitoguy]

The problem is that concurrent release builds will try to write to the master branch.
Whoever finishes last will get "git push" failure.

Options:

1. Disable parallel builds in CI settings.
2. Stop writing to master branch during release builds.

Evaluation:

1. We can disable parallel builds in Travis but it is done per whole repo. E.g. we cannot enable parallel PR builds and only serialize the release builds. Other CI systems may have better options here but we don't want a solution that requires specific CI. Also, disabling parallel builds slows down the builds.
2. We can stop writing to the master branch during releases:
   • avoid writing release notes to the file in the repo (instead write to GH releases, javadoc file, etc.)
   • avoid bumping "version.properties" (instead we can keep "2.1.*" in the file and smartly resolve the "*" version number. For example, resolve the version from the previous tag). This approach has an additional benefit - no version bump commits - some devs don't like it (I got this feedback from Pinot project - https://github.com/apache/incubator-pinot)

Thoughts?

PS. We might also just fix the tag issue as reported by cgtz. This is a resonable short term solution while we're working on 2)

[cgtz]

Hi @mockitoguy, thank you for looking into this.

For option 1., there may be still be some changes required within the shipkit plugin, since, if I'm correct, limiting the CI parallelism will not prevent new commits to the master branch, so the head may still change while performRelease is running. Does travis serialize the builds and keep the last build running, or does it immediately cancel the currently running build and start a new build on the current head?

For option 2., that may be nice to avoid producing extra commits, since for projects that generate a release on every commit, this can double the number of commits, but some other projects may also prefer to have the changelog checked in. Personally, I would rather not have the version bump commits, if possible (like pinot)

I agree that the intermediate solution of rolling back changes (tags, etc.) made to git on build failures, would help a lot.

[mockitoguy]

@cgtz, thank you for feedback!

[mockitoguy]

I agree that the intermediate solution of rolling back changes (tags, etc.) made to git on build failures, would help a lot.

I looked into that and I don't see any low hanging fruit worth implementing at this time. When 2 builds compete for the same resource (writable master) things are bound to get wrong. Long term we have a couple of options:

• use Git's "--atomic" flag on pushes
• Revert pushes when Bintray publication fails
• Do Bintray dryRun upload before pushing to Git

However, at this time I'd like to focus on eliminating the need to write to master.

[koral--]

Another theoretical option:
Introduce some kind of lock (eg. push tag release-<version>-lock) for the time of release process.
If release is in progress then subsequent requests can fail at the beginning or wait for finish (this would probably require polling).

[mockitoguy]

Introduce some kind of lock

Wow! Very creative idea :-) I think it is doable but I'm worried about the overall complexity of this solution. I suggest to push on the idea to avoid "release commits" on the master.

[mockitoguy]

@mockito/shipkit-developers, any thoughts/feedback about the proposed solution?