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

Skip testing for PR builds and non-lockfile push builds on greenkeeper branches #13025

Merged
merged 3 commits into from Jan 30, 2018
Merged

Skip testing for PR builds and non-lockfile push builds on greenkeeper branches #13025

merged 3 commits into from Jan 30, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jan 24, 2018

greenkeeper branches are currently tested on Travis four times.

  1. PR build for package.json update
  2. Push build for package.json update
  3. PR build for yarn.lock update
  4. Push build for yarn.lock update

The only case in which we need to run all the AMP tests is case 4.

This PR skips testing in cases 1, 2, and 3. The pre and post script steps that actually update the lockfile will be run normally.

This will reduce test time from ~20 min to ~1 min in cases 1, 2, and 3, , thereby reducing the overall Travis CPU usage.

@rsimha
Copy link
Contributor Author

rsimha commented Jan 24, 2018

/to @erwinmombay @cramforce

@cramforce
Copy link
Member

Shouldn't this be possible to configure in .travis.yml / Travis settings? I thought the branches->only only option did that.

@rsimha
Copy link
Contributor Author

rsimha commented Jan 25, 2018

@cramforce We'd like to run push builds on greenkeeper/* branches, but not PR builds. Removing this entry from branches->only will stop running push builds on greenkeeper branches. See https://docs.travis-ci.com/user/customizing-the-build/#Building-Specific-Branches

The Travis docs do not mention a way to specify that push builds be run for a particular branch, but not PR builds for PRs from that branch to master.

@erwinmombay
Copy link
Member

says in https://docs.travis-ci.com/user/conditional-builds-stages-jobs/ that you can do if: type IN (push, pull_request) (we can remove pull_request from the IN check)

@rsimha
Copy link
Contributor Author

rsimha commented Jan 25, 2018

That's promising, @erwinmombay :)

@rsimha
Copy link
Contributor Author

rsimha commented Jan 25, 2018

@erwinmombay @cramforce PTAL.

@rsimha
Copy link
Contributor Author

rsimha commented Jan 26, 2018

Bumping this.

@rsimha
Copy link
Contributor Author

rsimha commented Jan 26, 2018

@cramforce @erwinmombay From testing, it turns out that if a build is conditionally skipped, continuous-integration/travis-ci/pr, which is a required check, fails with the message The Travis CI build could not complete due to an error, and blocks the PR from being merged. Therefore, we can't conditionally cancel greenkeeper PR builds. I'm going to have to go back to my pr-check.js logic to skip all testing.

@rsimha rsimha changed the title Skip PR builds on greenkeeper branches Skip testing for PR builds and non-lockfile push builds on greenkeeper branches Jan 29, 2018
@rsimha
Copy link
Contributor Author

rsimha commented Jan 29, 2018

Tested this on an actual greenkeeper branch with push and PR builds, using lockfile and non-lockfile commits.

Non-lockfile commits:
PR: https://travis-ci.org/ampproject/amphtml/jobs/334940226#L1656 (skipped)
Push: https://travis-ci.org/ampproject/amphtml/jobs/334940209#L1672 (skipped)

Lockfile commits:
PR: https://travis-ci.org/ampproject/amphtml/jobs/334943871#L1665 (skipped)
Push: https://travis-ci.org/ampproject/amphtml/jobs/334943858#L1660 (run)

@rsimha rsimha merged commit 7836d38 into ampproject:master Jan 30, 2018
@rsimha rsimha deleted the 2018-01-24-SkipGreenkeeperPrBuilds branch January 30, 2018 00:15
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants