-
Notifications
You must be signed in to change notification settings - Fork 505
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
Make changes to Sigstore initialize #747
Conversation
f7021c8
to
301ac22
Compare
182246e
to
d02ca31
Compare
This is ready for review now |
panic("error creating root cert pool") | ||
} | ||
if err := tuf.GetTarget(ctx, fulcioTargetStr, &buf); err != nil { | ||
panic(errors.Wrap(err, "creating root cert pool")) |
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.
Is there a reason why we're using panics instead of errors in this package?
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.
This really shouldn't happen, but I'm not sure -- kind of was following existing code. We really should be getting these targets for any cosign verification, most of the callers expect these targets to be available.
I can plumb through errors, but it'll touch a bit of code
So sorry, I moved these files to keep them isolated. Can you rebase? |
updated! |
Looks like there's still a conflict :( |
d02ca31
to
2f75e70
Compare
because somehow i never pushed :P thanks! |
Looks like just a few lint errors now! |
2f75e70
to
ff9ee03
Compare
Signed-off-by: Asra Ali <asraa@google.com>
ff9ee03
to
c160e42
Compare
Signed-off-by: Asra Ali asraa@google.com
Summary
Ticket Link
Fixes
Release Note
cosign initialize
is explicitly run to check for updates (or a new root is provided)