Skip to content

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

Merged
ramonpetgrave64 merged 6 commits intoslsa-framework:mainfrom
ramonpetgrave64:fix-sign-attestations-schedule
Apr 24, 2024
Merged

fix: use @sigstore/cli in e2e.sign-attestations.schedule.yml#3572
ramonpetgrave64 merged 6 commits intoslsa-framework:mainfrom
ramonpetgrave64:fix-sign-attestations-schedule

Conversation

@ramonpetgrave64
Copy link
Contributor

@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.

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
Contributor Author

@laurentsimon @haydentherapper

node-version: 16
- name: install sigstore-js
run: npm ci
run: npm install -g @sigstore/cli@^0.8.0
Copy link
Collaborator

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
Contributor 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
Contributor 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.

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
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64 ramonpetgrave64 merged commit 8332e56 into slsa-framework:main Apr 24, 2024
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.

4 participants