Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Only trigger branch builds on master and release branches #14250

Merged
merged 2 commits into from
May 2, 2017
Merged

Only trigger branch builds on master and release branches #14250

merged 2 commits into from
May 2, 2017

Conversation

smashwilson
Copy link
Contributor

Description of the Change

Modify the Travis, Circle CI, and AppVeyor build metadata to only create branch builds for master and release branches named x.y-releases. This should help our build queue situation somewhat.

Note that pull request builds should still be triggered on all three systems.

Alternate Designs

We could do this by adding logic to the build scripts themselves to detect when they're being executed on a branch build that will also create a PR build, but there's no reason not to use the built-in solutions for this.

AppVeyor and (I believe) Circle both allow you to configure this setting in their own web UIs. I generally prefer to keep build configuration within the repository as much as possible, though, it's less confusing and we can manage it with pull requests (like this one!).

Why Should This Be In Core?

The core build metadata files are in core 😁

Benefits

This will decrease the extraneous builds that are created when maintainers push branches to the atom/atom fork and create pull requests from them, which is common in our workflow. This results in two builds for each push to a feature branch: one from the push to the feature branch and one from the pull request event:

screen shot 2017-04-18 at 12 54 21 pm

Running fewer builds will result in less queue pressure and more rapid feedback on changes.

Possible Drawbacks

Running the tests less often may cause us to miss flaky failures that happen only occasionally. We will also lose the ability to distinguish failures that are present on the branch itself (from the branch build) from those that are caused by the merge into master (from the pull request build).

Applicable Issues

N/A

We've agreed to hold off on changing the build for a day or so to more easily isolate the cause of any problems from #13646.

/cc @damieng @as-cii @maxbrunsfeld @50Wliu

@maxbrunsfeld
Copy link
Contributor

Thanks so much for taking care of this @smashwilson!

@smashwilson
Copy link
Contributor Author

It looks like this prevents the Circle build from running at all 🤔 I might have to turn that one back on.

@as-cii
Copy link
Contributor

as-cii commented Apr 20, 2017

Looks good after we restore CircleCI. Thanks, Ash! 👍

@smashwilson
Copy link
Contributor Author

We're getting a stacktrace generating the snapshot on AppVeyor:

Generating snapshot script at "C:\projects\atom\out\startup.js" (1573)AssertionError: 'CallExpression' == 'Identifier'
  at FileRequireTransform.replaceAssignmentOrDeclarationWithLazyFunction (C:\projects\atom\script\node_modules\electron-link\lib\file-require-transform.js:123:16)
  at Context.visitIdentifier (C:\projects\atom\script\node_modules\electron-link\lib\file-require-transform.js:72:18)
  at Context.invokeVisitorMethod (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:344:49)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:196:32)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at NodePath.each (C:\projects\atom\script\node_modules\ast-types\lib\path.js:101:26)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:219:18)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at Visitor.PVp.visit (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:133:29)
  at Object.visit (C:\projects\atom\script\node_modules\ast-types\lib\path-visitor.js:101:55)
  at FileRequireTransform.replaceDeferredRequiresWithLazyFunctions (C:\projects\atom\script\node_modules\electron-link\lib\file-require-transform.js:68:20)
  at FileRequireTransform.apply (C:\projects\atom\script\node_modules\electron-link\lib\file-require-transform.js:25:10)
  at C:\projects\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:54:54
  at undefined.next (native)
  at step (C:\projects\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:3:191)
  at C:\projects\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:3:361

@as-cii, any idea what that's about?

@smashwilson
Copy link
Contributor Author

Merging because the builds are correct now:

screen shot 2017-05-02 at 4 14 21 pm

@smashwilson smashwilson merged commit 6e2dcff into atom:master May 2, 2017
@smashwilson smashwilson deleted the aw-branch-builds branch May 2, 2017 20:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smashwilson @maxbrunsfeld @as-cii @benbalter @donokuda