Skip to content
This repository has been archived by the owner on Jan 18, 2021. It is now read-only.

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

Closed
epeee opened this issue Sep 9, 2017 · 16 comments
Closed

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

epeee opened this issue Sep 9, 2017 · 16 comments
Assignees
Labels

Comments

@epeee
Copy link
Contributor

epeee commented Sep 9, 2017

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:

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
Copy link
Member

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

@cgtz
Copy link

cgtz commented Dec 6, 2019

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
Copy link
Member

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

@mockitoguy mockitoguy self-assigned this Jan 8, 2020
@mockitoguy
Copy link
Member

mockitoguy commented Jan 10, 2020

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
Copy link

cgtz commented Jan 10, 2020

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
Copy link
Member

@cgtz, thank you for feedback!

@mockitoguy
Copy link
Member

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--
Copy link
Member

koral-- commented Jan 11, 2020

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
Copy link
Member

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
Copy link
Member

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

@mstachniuk
Copy link
Contributor

I like that release notes are kept in git history, even if they are also released to GitHub release page. Missing a possibility to disable parallel builds in Travis for release branches only is a huge gap for me. The workaround is using [ci skip-release] if you plan to merge more commits to the release branch in a short time and wait until builds are finished. But it doesn't scale well in bigger projects.
However, we can add some alternative to Shipkit, then users can choose what fits the best in their case.
Also this plugin: https://github.com/nemerosa/versioning (I'm using it at work + I'm working with the author) has a few interesting options how to bump a version. It can base on the latest tag and branch name - e.g. branch release/1.2 will publish 1.2.0, 1.2.1 etc.

@mockitoguy
Copy link
Member

plugin: https://github.com/nemerosa/versioning

This plugin looks interesting. Can you write a bit more how you use it?

@mockitoguy
Copy link
Member

For https://github.com/linkedin/ambry project I wrote a new simple & opinionated plugin: https://github.com/shipkit/shipkit-auto-version We will roll it out in Ambry and see how it works out.

@mstachniuk
Copy link
Contributor

plugin: https://github.com/nemerosa/versioning

This plugin looks interesting. Can you write a bit more how you use it?

We are using this plugin to release artifacts to the internal Nexus repository. We set usually: version = versioning.info.display in build.gradle and then each merge to release branch (e.g. release/2.0) will release 2.0.0, 2.0.1, 2.0.2, etc. There is no need to commit any changes after release because the version is calculated base on branch name and git tags.
We do not release usually from master, but if you would release also from master then:

if (versionInfo.branchType == "release") {
            version = versioning.info.display
        } else {
            version = (versioning.info.lastTag ?: "NO_TAG") + "-" + versioning.info.build
        }

The docs https://github.com/nemerosa/versioning are very good if you need more details.

@mockitoguy
Copy link
Member

Thank you for the details!

cgtz pushed a commit to linkedin/ambry that referenced this issue Feb 21, 2020
The goal is to release automatically from Travis and enable convenient merging of PRs, even if many PRs are merged concurrently (bug in Shipkit Gradle plugin: mockito/shipkit#395).
- instead of using Shipkit Gradle plugin we keep most CI/CD automation inside Ambry project. This way, it is easier to configure, debug and understand.
- we're using a new Gradle plugin "shipkit-auto-version". This plugin automatically deducts the version based on the most recent tag and the "version.properties" file. It is a very simple plugin, keeps the build logic simple, and keeps the Ambry build easy to maintain in the future.

This PR removes following features (acceptable trade-offs, we can implement it in the future):
- automated release notes generation
@mockitoguy
Copy link
Member

I will close this issue because the desired behavior can be achieved with https://github.com/shipkit/shipkit-auto-version

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants