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

[feature][nodejs] Support sha1 or tag for building #2138

Open
laurentsimon opened this issue May 18, 2023 · 6 comments
Open

[feature][nodejs] Support sha1 or tag for building #2138

laurentsimon opened this issue May 18, 2023 · 6 comments
Labels
area:nodejs Issue related to the Node.js builder

Comments

@laurentsimon
Copy link
Collaborator

See an example at https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#L315-L342

@laurentsimon laurentsimon added type:feature New feature or request status:triage Issue that has not been triaged area:BYOB An issue with the BYOB framework area:nodejs Issue related to the Node.js builder and removed type:feature New feature or request status:triage Issue that has not been triaged area:BYOB An issue with the BYOB framework labels May 18, 2023
@ianlewis
Copy link
Member

This example doesn't look to me to be anything unique to me. Was there something specific you meant?

This could be done via a run script.

npm i --prefer-online --no-fund --no-audit -g npm@latest

I'm not sure why they do this. IIUC is exactly what setup-node does except that it sets npm to read from the NODE_AUTH_TOKEN environment variable rather than setting a static value.

npm config set '//registry.npmjs.org/:_authToken'=\${PUBLISH_TOKEN}

They do a checkout a ref from earlier in the workflow so maybe that's something we do need to support. This seems like a similar use case to JReleaser where they create a new commit for the release and check that out.

While I don't really want to require that folks change their workflows, I feel like the proper thing to do is actually trigger a separate workflow run based on that commit. This is like what changesets/action does. It creates a PR for the release which can trigger its own workflows when it's merged. We've already encountered some difficulties with verification (e.g. slsa-framework/slsa-verifier#599)

@ianlewis
Copy link
Member

Ah I see, as far as the install goes, it kind of looks like they were installing the latest npm just so they could use the --provenance flag but I'm still not sure why they didn't just rely on setup-node to do the setup for them?

@laurentsimon
Copy link
Collaborator Author

Sorry, I should have been clearer. The line to look at is https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#L330. It's using a tag that seems to be created in an earlier job https://github.com/npm/node-semver/blob/main/.github/workflows/release.yml#LL60C28-L60C28 (I've not verified the end-to-end call, though), instead of using the tag / sha from the GitHub event. (Note: the event is workflow_dispatch, so there is not tag by default)

@ianlewis
Copy link
Member

Yeah, I noticed some workflows do this. I was kind of hoping we could get folks to adopt a model where they don't create new commits and build from them in the same workflow run.

Rather, they could create a PR for the release and/or trigger a new workflow run for the new commit. This is what the changesets tool does. Example PR: sigstore/sigstore-js#489

@laurentsimon
Copy link
Collaborator Author

laurentsimon commented May 23, 2023

Agreed no pushing directly to the repo is better. I suspect it's more work for users to change their setup, and it will be an adoption problem. But we'll see.

@ianlewis
Copy link
Member

ianlewis commented May 23, 2023

Agreed no pushing directly to the repo is better. I suspect it's more work for users to change their setup, and it will be an adoption problem. But we'll see.

Yeah, I suppose we will need to support it at some point but I'd like folks to not do it if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:nodejs Issue related to the Node.js builder
Projects
None yet
Development

No branches or pull requests

2 participants