-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
chore: fix npm-publish dependencies and add provenance #3099
Conversation
db6082d
to
3f02e7a
Compare
@nodejs/node-gyp would you mind taking a look at this so that we can release v10.3.0? Thank you! |
@@ -19,7 +19,8 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v4 | |||
- run: pip install --user ruff | |||
- run: ruff check --output-format=github --select="E,F,PLC,PLE,UP,W,YTT" --ignore="E721,PLC1901,S101,UP031" --target-version=py38 . | |||
# Excluding `/gyp` directory as it is been checked in https://github.com/nodejs/gyp-next/ already |
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.
This assumption does not make me comfortable. When you vendor-in source code, it becomes your code, and you should run your own tests on it.
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.
Can we run two separate checks? It would be great to surface issues in /gyp
because we do vendor it. But in the past it has created friction because a linting error in /gyp
code would fail all of 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.
@legendecas Can we revert this part and re-open as a new PR? It would be nice to unblock the release from CI and figure out python linting in a separate discussion.
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.
This is added because the ruff check
is failing in this repo: https://github.com/nodejs/node-gyp/actions/runs/12087642318/job/33709509643. But the ruff check
is not failing on the gyp-next repo.
There are differences on the ruff configurations between the two repos. I'm fine with reverting this one but we will have to merge this PR with failing CIs.
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.
Thanks for the explanation @legendecas, I didn't realize CI was failing. I think this change should be landed to make CI green and a new issue/PR opened to get two different ruff
checks running, each with the correct configuration.
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.
Approved but I think the ruff check
changes should move to a separate PR discussion.
057d4ee
to
b2f127c
Compare
b2f127c
to
4f6fdae
Compare
The
npm-publish
steps depends on output variablerelease-please.outputs.release_created
. Without this variable the steps are skipped, e.g. https://github.com/nodejs/node-gyp/actions/runs/12087642317.Adds npm provenance statements: https://docs.npmjs.com/generating-provenance-statements#example-github-actions-workflow.
The release-please bot created PR needs an additional token to trigger CI runs:
Removing the always skipped release tests for now.