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

Breaking: separate releases into "generate" and "publish" phase #27

Merged
merged 5 commits into from Oct 10, 2018

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Sep 28, 2018

This updates eslint-release to have separate commands for generating a release locally, and publishing info about the release to various locations (npm, git, GitHub releases). In addition to making it easier to deal with failures (since we won't end up in a half-published state), this will also make it possible to provide a 2FA code for publishing that will still be valid when it's actually time to publish.

To make future changes easier, this also removes some functionality that is no longer used (e.g. the distinction between CI and non-CI releases, and the ability to publish the changelog to GitHub changelog separately from the rest of the release)

Migration for existing projects:

  • Projects that run the eslint-release command should run eslint-generate-release followed by eslint-publish-release.
  • Projects that run the eslint-prerelease <releaseId> command should run eslint-generate-prerelease <releaseId> followed by eslint-publish-release.
  • Projects that use ReleaseOps.release(releaseInfo) should run ReleaseOps.generateRelease(releaseInfo) followed by ReleaseOps.publishRelease().
  • Projects should add .eslint-release-info.json to .gitignore.

When publishing, an NPM_OTP environment variable can optionally be used to authenticate the publish.

Refs: eslint/eslint#10631

This updates `eslint-release` to have separate commands for generating a release locally, and publishing info about the release to various locations (npm, git, GitHub releases). In addition to making it easier to deal with failures (since we won't end up in a half-published state), this will also make it possible to provide a 2FA code for publishing that will still be valid when it's actually time to publish.

To make future changes easier, this also removes some functionality that is no longer used (e.g. the distinction between CI and non-CI releases, and the ability to publish the changelog to GitHub changelog separately from the rest of the release)

Migration for existing projects:

* Projects that run the `eslint-release` command should run `eslint-generate-release` followed by `eslint-publish-release`.
* Projects that run the `eslint-prerelease <releaseId>` command should run `eslint-generate-prerelease <releaseId>` followed by `eslint-publish-release`.
* Projects that use `ReleaseOps.release(releaseInfo)` should run `ReleaseOps.generateRelease(releaseInfo)` followed by `ReleaseOps.publishRelease()`.

When publishing, an `NPM_OTP` environment variable can optionally be used to authenticate the publish.
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Just left one question about the temporary file.

// Release needs a .npmrc file to work properly - token is read from environment
console.log("Writing .npmrc file");
fs.writeFileSync(".npmrc", "//registry.npmjs.org/:_authToken=${NPM_TOKEN}");
fs.writeFileSync(".eslint-release-info.json", JSON.stringify(releaseInfo));
Copy link
Member

Choose a reason for hiding this comment

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

As a separate item, should we add .eslint-release-info.json to all ESLint organization repositories' .gitignore file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. I'll add that to the "migration guide" at the top of this PR.

The npm publish is generally more likely to fail than the git push (e.g. as a result of making a typo in a OTP). This updates the script to try publishing to npm first, to avoid creating an inconsistent state if the npm publish fails.
This should make it slightly easier to figure out what's going on if any issues need to be debugged.
@not-an-aardvark
Copy link
Member Author

FYI, I pushed a couple minor changes that should help with debugging if something goes wrong. Feel free to review again if you'd like, otherwise I'll plan to merge this a bit later today.

@not-an-aardvark not-an-aardvark merged commit 9d0445a into master Oct 10, 2018
@not-an-aardvark not-an-aardvark deleted the no-more-ci-release branch October 10, 2018 21:57
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.

None yet

2 participants