-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add image attestation workflow step #5158
Conversation
Signed-off-by: Dan Urson <dan.urson@cresta.ai>
Looks like this needs some additional permissions per step one of the action's docs. |
I can't see the vuln that Fossa's complaining about. @nitrocode can you take a look? |
Thanks for the contribution! Could you test this out in your fork by merging your |
the vulnerbility issue should be fixed per #5165 |
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.
lgtm
@nitrocode @chenrui333 Friends, thank you for your patience. We conducted some additional tests and were able to successfully sign/attest to an image in my fork. I would appreciate it if you could review the most recent commit here, and give it a maintainer-grade 👍 if you're okay with it. |
@jamengual ok - it's still failing, but that's expected. it's because we're hitting this issue in the attest-build-provenance action - tl;dr, the action won't run if it's run as part of a merge from a fork, the merge has to be completed from a branch in the same repo. in my fork, the attestation works on the latest build. are you comfortable approving a merge with this in mind? |
The image building is expected to fail because the github token changes are not merged to main yet. |
@notdurson question, I saw we were gating the attest and push on the global PUSH var. Why was it removed? |
@GenPage I removed it because it was causing the attestation step to skip during test builds. I can include dependency updates in tests in the future, in order to trigger a push; however, this might fail due to my lack of member/maintainer permission. It'll also slow down the process. Would you like me to add the gate back, or are you okay with leaving it out? |
@notdurson Ah, so it was for testing. Our main image has a gate for pushing where we want to test builds and not push (on PRs). Let's add it back for consistency unless I misunderstand the attestation point. It should only be required for images pushed not solely built correct? |
@GenPage Sounds good, incoming! |
fe8e19d
to
20916e8
Compare
Signed-off-by: Dan Urson <dan.urson@cresta.ai>
@GenPage @jamengual apologies for delay. I cleaned the tree and force-pushed the relevant changes. |
what
why
tests
cosign
run was successful.references