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

ci: release postject to npm on ci #30

Closed
wants to merge 2 commits into from

Conversation

RaisinTen
Copy link
Contributor

Fixes: #14
Signed-off-by: Darshan Sen raisinten@gmail.com

@RaisinTen RaisinTen force-pushed the release-postject-to-npm-on-ci branch from 7ac857d to e15a9a2 Compare September 26, 2022 07:33
@RaisinTen RaisinTen marked this pull request as draft September 26, 2022 12:14
@RaisinTen RaisinTen force-pushed the release-postject-to-npm-on-ci branch from f003901 to 9054aa2 Compare September 26, 2022 13:27
@RaisinTen RaisinTen marked this pull request as ready for review September 26, 2022 13:27
Fixes: #14
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the release-postject-to-npm-on-ci branch from 9054aa2 to 78837e3 Compare September 27, 2022 10:58
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the release-postject-to-npm-on-ci branch from 78837e3 to a8bae4c Compare September 27, 2022 14:20
@dsanders11
Copy link
Contributor

It seems like this will publish every commit to NPM, since it's just a new CI job?

Copy link
Contributor

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

I think we should put a pin in this for the moment and stick to manually releasing for our immediate needs. There's a lot of sharp edges I want to make sure we wire up correctly so we shouldn't rush it.

Issues with the current state of this PR:

  • It is publishing from all branches on all commits. That means this PR has actually published two releases while in PR state.
  • It should do fan-in on the test jobs, otherwise it will publish releases only when the tests pass on the linux executor, regardless of the state of the other jobs - we want publish only on all green.
  • I believe the current setup means if I re-run the job with SSH access, I can go read out your personal NPM token.

Let's consider using semantic-release to make a more robust process and avoid the sharp edges. We can also evaluate @continuous-auth/semantic-release-npm for 2FA goodness.

## Install

```sh
npm i postject
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
npm i postject
npm i -g postject

There's a good chance users of the Postject CLI will be using it outside of a Node project, so I think installing it globally is the better option.

Comment on lines +277 to +279
matrix:
parameters:
os: [ linux ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a matrix here, or parameters for the publish command further up in this config? We're just using the linux executor so we should be able to just use that one explicitly.

@RaisinTen
Copy link
Contributor Author

As discussed in DMs, we'll stick to doing releases manually.

My main use for doing NPM releases on a PR was that it's easier to test changes that have not been merged on CI in other projects by just updating the package version but the solution we decided on was:
a. test such changes locally
b. publish such packages under personal namespaces and use that

@RaisinTen RaisinTen closed this Sep 28, 2022
@RaisinTen RaisinTen deleted the release-postject-to-npm-on-ci branch September 28, 2022 06:07
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.

Release an NPM package for Postject
2 participants