Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(travis): prevent double builds on PRs #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tlvince
Copy link
Contributor

@tlvince tlvince commented Apr 24, 2018

Without this, if a PR is opened for a branch, Travis creates two builds;
one for the branch and one for the PR itself. When Travis is configured
(in the UI) to build both the push and PR (the default), I think a
better default for builds is to trigger only one build.

Semantic release itself uses this set up, see:
semantic-release/commit-analyzer#11

Without this, if a PR is opened for a branch, Travis creates two builds;
one for the branch and one for the PR itself. When Travis is configured
(in the UI) to build both the push and PR (the default), I think a
better default for builds is to trigger only _one_ build.

Semantic release itself uses this set up, see:
semantic-release/commit-analyzer#11
// ignore git tags created by semantic-release, like "v1.2.3"
except: [/^v\d+\.\d+\.\d+$/.toString()],
// Avoid double build on PRs (see: https://github.com/travis-ci/travis-ci/issues/1147)
only: ['master'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do something similar on my own repositories, e.g. https://github.com/octokit/rest.js/blob/aa816a6ea086b0d6ea39ca77845c345c3151cf5a/.travis.yml#L7-L12

But it’s a potential pitfall if you don’t know what you are doing. We had discussions in the past to put in more "best practises" into the .travis.yml file but mostly decided to keep it simple to reduce the support load for us when there are no builds. E.g. here the won’t be any builds if the repositorie’s main branch is not configured to master. And Greenkeeper won’t be able to check for breaking changes in the background as it needs to be able to run builds for branches without pull reuqests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a fair point. I've always treated semantic-release-cli's output as a starting point and make changes afterwards. I guess the point here is deciding what's considered "good defaults". master is the default release branch, so this would work by default.

On the other hand, maybe this is surprising and is better suited to a "travis tips" section in the docs. Either way, happy to err on the side of whatever's least of a burden for y'all :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both solution (except : [^v\d+\.\d+\.\d+$] and only: [master]) would create a problem when semantic-release/semantic-release#563 lands as we would release from multiple branches.
We had several debates regarding what to recommend in the cli and in the doc regarding "best practice". We decided to recommend the least possible as it creates a lot of (sometimes conflictual/passionate) debate as it seems to be a subject on which everyone has an opinion.

So I would rather not make any recommendation, not even in the doc.
What we can do though is to link blog article in docs/resources.md.

For this PR I think we should just remove branch altogether.

@pvdlg pvdlg added the cli label May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants