-
-
Notifications
You must be signed in to change notification settings - Fork 235
ci(release): npm install
in test_node.yml
on release
#2768
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): npm install
in test_node.yml
on release
#2768
Conversation
1aefb76
to
40bb062
Compare
@@ -0,0 +1,22 @@ | |||
#!/bin/bash | |||
|
|||
# This script is run by Craft after a release is created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
40bb062
to
b1d8933
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to make a release to properly test this. I plan to do immediately after merging
npm install
in bump-version.sh
npm install
in test_node.yml
on release
You can see the checks passing on this release, which was generated against this PR. |
- run: SENTRYCLI_SKIP_DOWNLOAD=1 npm ci | ||
- name: Install dependencies via npm ci | ||
if: ${{ !inputs.triggered-by-release }} | ||
run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we previously just do SENTRYCLI_SKIP_DOWNLOAD=1 npm install
? Seemed a bit simpler than the check and the conditional steps 😅
No need to change if this works, was just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Also, do we still need the SENTRYCLI_SKIP_DOWNLOAD
env variable check at all anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we previously just do SENTRYCLI_SKIP_DOWNLOAD=1 npm install? Seemed a bit simpler than the check and the conditional steps 😅
Yeah, I think that is why it broke now, but at the same time, I want the locking to avoid situations like we had a few days ago, with incompatible releases breaking our CI. Better to run CI against locked dependencies to make it reproducible, imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the environment variable is unnecessary when providing --ignore-scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably could remove the check, only problem is we advertise it as public API in the README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, didn't know this was mentioned in documentation. then let's leave it as-is. Thanks!
b1d8933
to
c37e733
Compare
`npm ci` will fail here, as the new versions of the optional dependencies are not published yet. Additionally, add a script to bump the optional dependencies in the package-lock.json after a release is created. Otherwise, `npm ci` will continue to fail after the release, until someone updates the package-lock.json manually.
c37e733
to
9826efe
Compare
`npm ci` will fail here, as the new versions of the optional dependencies are not published yet. Additionally, add a script to bump the optional dependencies in the package-lock.json after a release is created. Otherwise, `npm ci` will continue to fail after the release, until someone updates the package-lock.json manually.
npm ci
will fail here, as the new versions of the optional dependencies are not published yet.Additionally, add a script to bump the optional dependencies in the package-lock.json after a release is created. Otherwise,
npm ci
will continue to fail after the release, until someone updates the package-lock.json manually.