-
Notifications
You must be signed in to change notification settings - Fork 130
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
Do not initialize Rekor client if transparency is not enabled. Issue #520 #693
Conversation
The following is the coverage report on the affected files.
|
pkg/chains/rekor.go
Outdated
@@ -68,8 +68,10 @@ func publicKeyOrCert(signer signing.Signer, cert string) ([]byte, error) { | |||
return pem, nil | |||
} | |||
|
|||
// for testing | |||
var getRekor = func(url string, l *zap.SugaredLogger) (rekorClient, error) { | |||
var getRekor = func(is_enabled bool, url string, l *zap.SugaredLogger) (rekorClient, error) { |
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.
Would it be more straight forward to only call getRekor
if it is enabled since the caller already has that information?
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.
Cool! I updated the PR and moved the client initialization to be gated inside the shouldUploadTlog function. Let me know if this looks good to you. Thanks!
The following is the coverage report on the affected files.
|
6be786b
to
1b1466e
Compare
The following is the coverage report on the affected files.
|
1b1466e
to
41e093f
Compare
8d977e4
to
4de42ff
Compare
The following is the coverage report on the affected files.
|
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.
Just a small nit.
pkg/chains/signing.go
Outdated
if err != nil { | ||
return err | ||
} | ||
var rekorClient rekorClient |
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 benefit to declaring this variable this far from when it's actually used?
It could be useful to only have to call getRekor
once during this method, but that doesn't seem to be the case here.
9fc5da7
to
9020a8d
Compare
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
/retest |
Moved the initalization of rekorClient to be gated inside the shouldUploadTlog function to ensure transparency is enabled
9020a8d
to
55975ce
Compare
FYI @lcarva: It looks like one the E2E tests is flaky and the only way to trigger retesting was to submit a new PR so I just fixed a typo in the readme and the tests are now passing. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcarva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Bummer... If you managed to save some details about the failure, could you file an issue here? 🙏 |
Sure, I opened an issue for this here |
Changes
Issue #520
Moved the initalization of rekorClient to be gated inside the shouldUploadTlog function to ensure transparency is enabled
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes