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

refactor: change order for publishing #6

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Conversation

nfischer
Copy link
Member

This completely changes the order of steps we follow for publishing a
package. The goal here is to move npm publish as close as possible to
the beginning so that we can error-out early if it fails (e.g. 2FA
errors).

At a high level, our new steps are:

  • npm version (bump version, commit, make tag)
  • echo npm publish
    • if successful, push commit & tag
    • if failed, undo commit & tag

In the case of failure, we still provide the same granularity of error
output: we check to see if the user is logged in, if they're
owner/contributor, etc. In addition, we also provide some error output
if the user is outside of a git repo (the usual cause for npm version
failures).

While this refactor is done to enable supporting 2FA, this does not
itself provide OTP support. This refactor also adds TODOs for other
bugs, which will be resolved in follow-up PRs.

Issue #5

This completely changes the order of steps we follow for publishing a
package. The goal here is to move `npm publish` as close as possible to
the beginning so that we can error-out early if it fails (e.g. 2FA
errors).

At a high level, our new steps are:

 * npm version (bump version, commit, make tag)
 * echo npm publish
   * if successful, push commit & tag
   * if failed, undo commit & tag

In the case of failure, we still provide the same granularity of error
output: we check to see if the user is logged in, if they're
owner/contributor, etc. In addition, we also provide some error output
if the user is outside of a git repo (the usual cause for `npm version`
failures).

While this refactor is done to enable supporting 2FA, this does not
itself provide OTP support. This refactor also adds TODOs for other
bugs, which will be resolved in follow-up PRs.

Issue #5
@nfischer
Copy link
Member Author

@freitagbr this is battle tested on the v0.2.0 releases for the plugins:

This finishes way faster now (about 6-7 seconds). I did local tests with an extra patch for --otp which will wrap up #5, but I'll upload that in a separate PR.

Copy link

@freitagbr freitagbr left a comment

Choose a reason for hiding this comment

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

We can fix issue #4 while we're here.

config.silent = false;
config.fatal = true;
try {
// TODO(nfischer): only allow releases from master branch (issue #4)

Choose a reason for hiding this comment

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

This can be checked easily:

if (exec('git rev-parse --abbrev-ref HEAD').trim() !== 'master') {
    echo('');
    echo('Releases are permitted only from the master branch.');
    exit(1);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it in a separate PR

@freitagbr
Copy link

LGTM

@nfischer
Copy link
Member Author

Thanks! I'll upload PRs later to finish #5 and fix the TODOs for #4 and #2

@nfischer nfischer merged commit 51c8a75 into master Jan 30, 2018
@nfischer nfischer deleted the refactor-change-order branch January 30, 2018 21:46
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.

2 participants