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

[nan-668] add verification bash script to publish packages #1930

Conversation

khaliqgant
Copy link
Member

@khaliqgant khaliqgant commented Apr 1, 2024

Describe your changes

The goal with this PR is to mimic a publish process of the cli with the code that is in review to ensure a working cli that is in a remote source. This is accomplished by building and publishing the packages to the Github registry and finally using the cli to verify that it works expected. This will protect against instances where a faulty cli is published. This will be especially useful to verify that this PR: #1918 works as expected with packaging the utils package. Additionally, as we refactor the cli to move code out of shared and reduce the package size of the cli this will ensure things still work as expected.

Issue ticket number and link

NAN-668

Checklist before requesting a review (skip if just adding/editing APIs & templates)

  • I added tests, otherwise the reason is:
  • I added observability, otherwise the reason is:
  • I added analytics, otherwise the reason is:

Copy link

linear bot commented Apr 1, 2024

.github/workflows/cli-verification.yaml Outdated Show resolved Hide resolved
scripts/publish.sh Outdated Show resolved Hide resolved
NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
jq '.name = "@nangohq/cli"' package.json > temp.json && mv temp.json package.json
npm publish --access public
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't the cli published by running the publish.sh script in the previous step? why do we need to publish it again?

Copy link
Member Author

Choose a reason for hiding this comment

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

It attempts to publish it but fails because it is not scoped under our organization @nangohq which is why I rename the package and publish it separately.

- name: Publish the cli privately under the correct scope
working-directory: packages/cli
env:
NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
jq '.name = "@nangohq/cli"' package.json > temp.json && mv temp.json package.json
npm publish --access public

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not doing that directly then?
Also doesn't it will publish @nangohq/node and @nangohq/shared every time for nothing?

Copy link
Member Author

Choose a reason for hiding this comment

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

why not doing that directly then?

Easier to reuse the script which bumps the references and packages correctly.

Also doesn't it will publish @nangohq/node and @nangohq/shared every time for nothing?

Shared needs to be published, node is published again and not used as is frontend but there is no harm in publishing those packages.

@khaliqgant khaliqgant requested a review from TBonnin April 3, 2024 06:27
.github/workflows/cli-verification.yaml Show resolved Hide resolved
NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
jq '.name = "@nangohq/cli"' package.json > temp.json && mv temp.json package.json
npm publish --access public
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not doing that directly then?
Also doesn't it will publish @nangohq/node and @nangohq/shared every time for nothing?

@khaliqgant khaliqgant merged commit d3c521a into master Apr 3, 2024
19 checks passed
@khaliqgant khaliqgant deleted the khaliq/nan-668-add-cli-test-to-ensure-bad-versions-are-published-that-wont branch April 3, 2024 17:56
@TBonnin
Copy link
Collaborator

TBonnin commented Apr 3, 2024

It is a bit of an hacky solution but since it is contained in a single github actions I am fine with it. We can improve it in the future if the need arises

@khaliqgant
Copy link
Member Author

It is a bit of hacky solution but since it is contained in a single github actions I am fine with it. We can improve it in the future if the need arises

Agree it is a bit (read: very) hacky but also agree if it causes problems we can improve or rip it out even. The hope is that it saves us in the future. Time will tell if that is actually the case.

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.

3 participants