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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/travis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ const travisyml = {
node_js: ['9', '8', '6', '4'], // eslint-disable-line camelcase
after_success: ['npm run travis-deploy-once "npm run semantic-release"'], // eslint-disable-line camelcase
branches: {
// 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.

Yeah maybe we could add tips to https://semantic-release.gitbooks.io/semantic-release/docs/recipes/travis.html and https://semantic-release.gitbooks.io/semantic-release/docs/recipes/travis-build-stages.html?

I would probably add both only: ['master'], as well as the config at https://github.com/octokit/rest.js/blob/aa816a6ea086b0d6ea39ca77845c345c3151cf5a/.travis.yml#L7-L12 as many folks use semantic-release together with greenkeeper

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.

},
};

Expand Down