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

Add digest input to container docs #591

Merged
merged 2 commits into from
Jul 22, 2022

Conversation

ianlewis
Copy link
Member

Fixes #543

@@ -134,6 +136,7 @@ jobs:
uses: slsa-framework/slsa-github-generator/.github/workflows/generator_container_slsa3.yml@main
with:
image: ${{ needs.build.outputs.image }}
digest: ${{ needs.build.outputs.digest }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

have we decided yet if image and digest should be separate inputs or not?
Fo example, in https://github.com/actions/starter-workflows/blob/main/ci/docker-publish.yml#L93, it's passed as one parameter.

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 kind of wanted to have them be separate to make it clear that only the digest is supported. I wanted to force folks to use the digest to avoid racing with something that could modify the image tag.

I could merge them into one and do some validation on the image string to make sure it includes the digest and return an error if not, but I thought that this would better UX. wdut?

Copy link
Collaborator

@laurentsimon laurentsimon Jul 20, 2022

Choose a reason for hiding this comment

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

I'm not a UX expert tbh. I think it'd be fine to have a single option. So long as it's a required option and we validate it, it's fine. You may be right that it makes the hash more explicit.

We can re-visit later without blocking this PR for now.

@asraa @joshuagl do you have any thoughts on the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

SG. Agreed that it might not be a nicer UX, but a more explicit one.

@ianlewis ianlewis enabled auto-merge (squash) July 22, 2022 02:17
@ianlewis ianlewis merged commit e49ee2a into slsa-framework:main Jul 22, 2022
@ianlewis ianlewis deleted the container-doc-digest branch July 27, 2022 01:20
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.

[bug] Missing input digest in readme example for container workflow
2 participants