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

fix: use @sigstore/cli in e2e.sign-attestations.schedule.yml #3572

Conversation

ramonpetgrave64
Copy link
Collaborator

@ramonpetgrave64 ramonpetgrave64 commented Apr 16, 2024

Summary

Addresses #3002

Fixes the .github/workflows/e2e.sign-attestations.schedule.yml workflow.
sigstore-js now has its cli tools in a separate package, to be installed with install -g @sigstore/cli.

Testing Process

Invoked the workflow from my personal fork

Verification succeeded
Verification succeeded

We can't add this to a pre-submit, because it requires token permissions that are not available to forks' PR runs.

Checklist

  • Review the contributing guidelines
  • Add a reference to related issues in the PR description.
  • Update documentation if applicable.
  • Add unit tests if applicable.
  • Add changes to the CHANGELOG if applicable.

Sorry, something went wrong.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64 ramonpetgrave64 marked this pull request as ready for review April 16, 2024 20:50
@ramonpetgrave64
Copy link
Collaborator Author

@@ -44,13 +44,13 @@ jobs:
with:
node-version: 16
- name: install sigstore-js
run: npm ci
run: npm install -g @sigstore/cli@^0.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Does npm have a concept of latest? Then you don't have to worry about updating to the latest sigstore-js CLI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we could always install the latest

npm install -g @sigstore/cli

, though we generally like to pin versions when we can. But I guess if the signer's version is already pinned, it could be okay to leave the verifier unpinned. @ianlewis wdyk?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep pinning, this is useful for determinism. A better way to do to use a lock file, so that dependabot / renovatebot sends us PR. renovatebot supports custom regex to let it know it should interpret this ^0.8.0 as a version to update. I think it's easier to use a lock file to pin the dep.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we don't pin in the package.json in the project root? I managed the versions for dev dependencies there so that renovate could send PRs to update them etc.

https://github.com/slsa-framework/slsa-github-generator/blob/main/package.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that's a better place for this.

@@ -44,13 +44,13 @@ jobs:
with:
node-version: 16
- name: install sigstore-js
run: npm ci
run: npm install -g @sigstore/cli@^0.8.0
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we don't pin in the package.json in the project root? I managed the versions for dev dependencies there so that renovate could send PRs to update them etc.

https://github.com/slsa-framework/slsa-github-generator/blob/main/package.json

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
ramonpetgrave64 and others added 2 commits April 24, 2024 16:08

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64 ramonpetgrave64 merged commit 8332e56 into slsa-framework:main Apr 24, 2024
74 checks passed
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.

None yet

4 participants