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

Trigger canary release on successful PR build #246

Merged
merged 56 commits into from Jul 13, 2017
Merged

Trigger canary release on successful PR build #246

merged 56 commits into from Jul 13, 2017

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Jul 7, 2017

Update from @shawnbot

This PR adds some travis-ci config options that will run script/cibuild after the build succeeds. If this is a PR build (when TRAVIS_EVENT_TYPE = pull_request), then we publish a canary release to the canary npm dist-tag.

We've stubbed out publish commands for release candidates and final releases, but for now those commands are just echoed rather than executed.

We've also eliminated our per-module build matrix and now run the tests for all modules in one go, which should speed up testing time significantly.

@jonrohan jonrohan requested a review from shawnbot July 7, 2017 20:45
.travis.yml Outdated
@@ -2,6 +2,8 @@ language: node_js
node_js:
- '7'
env:
global:
- COMMIT=${TRAVIS_COMMIT::8}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, does this modify the build matrix? If so, maybe we just do this when we generate the prerelease id.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it changed the matrix, https://travis-ci.org/primer/primer-css/builds/251305487

Looks like it just included a $COMMIT variable in all the builds

@jonrohan jonrohan changed the base branch from master to dev July 7, 2017 21:32
"force": "lerna publish --force-publish=*"
"updated": "lerna updated",
"release": "lerna publish",
"release-candidate": "lerna publish --npm-tag=rc",
Copy link
Member

Choose a reason for hiding this comment

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

FYI keeping the --skip-git on release-candidate keeps the version changes locally but doesn't publish them, so when you go from release candidate to a release you don't have to force publish. If we don't do this then we'll need to add a force publish option to the scripts which I'm also not sure why you've removed in this pr. It also means we have more force publish options to consider as right now the forma script publishes every single package which we might not want to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my thinking was that we don't need --skip-git because these commands will be run on Travis, from whence we we won't commit changes or push tags.

"rc": "lerna publish --npm-tag=rc --skip-git",
"force": "lerna publish --force-publish=*"
"updated": "lerna updated",
"release": "lerna publish",

Choose a reason for hiding this comment

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

So a release will be lerna publish --yes

How does that resolve versions?

e.g.

a@1.1.0

b@2.0.0 depends on a@^1.1.0

c@3.0.0 depends on a@^1.1.0

d@4.0.0

If you merged changes to master that constitute a minor bump of:

a@1.1.0 to a@1.2.0

  • causes b@2.0.0 to become b@2.0.1
  • causes c@3.0.0 to become c@3.0.1

and a major of:

d@4.0.0 to d@5.0.0

as well as a small change of:

c@3.0.0 to c@3.0.1

How would that all get resolved non interactively?

The result needs to be:

a@1.2.0
b@2.0.1
c@3.0.1
d@5.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for weighing in, @bradbarrow. My hope – perhaps based on bad assumptions, which I'm more than happy to reassess – was that we could do the version resolution with lerna publish --no-npm and file a PR with the resulting versions, then have those versions published without any incrementing or prompting when the PR merges. I haven't tested this out yet, but I plan to today.

@shawnbot
Copy link
Contributor

@jonrohan @broccolini How would you all feel about scoping this down to publish canary releases whenever a PR build succeeds? Then we could at least test that feature on this PR before merging.

@shawnbot
Copy link
Contributor

Status update:

I cherry-picked 871d806 so the tests would pass, and this build succeeded... but then failed because npm failed to authenticate. So I replaced the NPM_API_KEY env var on Travis with mine, and restarted the build. 🤞

@shawnbot
Copy link
Contributor

Welp, that failed. I think I can get it to work by removing the NPM_EMAIL env var, which is probably confusing npm.

@shawnbot
Copy link
Contributor

Hey, it worked! The PR build published the merge commit a7f1f71 as primer-css@9.1.0-alpha.a7f1f710, and the push build did nothing. Here's the published CSS. ✨

@broccolini
Copy link
Member

How would you all feel about scoping this down to publish canary releases whenever a PR build succeeds? Then we could at least test that feature on this PR before merging.

I'm okay with that.

@shawnbot
Copy link
Contributor

Sweet, so I'll merge this, then we can do the commit status updates in another PR! ✨

# merges to dev build a release candidate
elif [[ $TRAVIS_BRANCH = dev ]]; then
echo "👌 Publishing release candidate..."
echo npm run release-candidate -- ${args}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note the echo here and on line 19 will just output the command rather than run it. We'll need to remove those as we're ready to test those release stages.

@shawnbot shawnbot changed the title [WIP] Trigger dev module publishing on successful build Trigger canary release on successful PR build Jul 13, 2017
Copy link
Member

@broccolini broccolini left a comment

Choose a reason for hiding this comment

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

Thanks ya'll, great to see this progressing 😎

@shawnbot shawnbot merged commit 328a0a4 into dev Jul 13, 2017
@shawnbot shawnbot deleted the after_success branch July 13, 2017 20:07
This was referenced Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants