Skip to content

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Sep 18, 2025

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.

@szokeasaurusrex szokeasaurusrex requested a review from a team as a code owner September 18, 2025 10:07
@szokeasaurusrex szokeasaurusrex marked this pull request as draft September 18, 2025 10:07
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/npm-install-in-bump-version branch 7 times, most recently from 1aefb76 to 40bb062 Compare September 18, 2025 14:03
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review September 18, 2025 14:04
@@ -0,0 +1,22 @@
#!/bin/bash

# This script is run by Craft after a release is created.
Copy link
Member Author

Choose a reason for hiding this comment

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

See here and here for more info

cursor[bot]

This comment was marked as outdated.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/npm-install-in-bump-version branch from 40bb062 to b1d8933 Compare September 18, 2025 14:11
Copy link
Member Author

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

@szokeasaurusrex szokeasaurusrex changed the title ci(release): npm install in bump-version.sh ci(release): npm install in test_node.yml on release Sep 18, 2025
@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Sep 18, 2025

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
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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!

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/npm-install-in-bump-version branch from b1d8933 to c37e733 Compare September 18, 2025 14:31
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) September 18, 2025 14:31
`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.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/npm-install-in-bump-version branch from c37e733 to 9826efe Compare September 18, 2025 14:33
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) September 18, 2025 14:33
@szokeasaurusrex szokeasaurusrex merged commit 9d49ba9 into master Sep 18, 2025
25 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/npm-install-in-bump-version branch September 18, 2025 14:36
runningcode pushed a commit that referenced this pull request Sep 19, 2025
`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.
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