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

Enable parsing of incomplete minisign keys, to enable re-indexing. #567

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

dlorenc
Copy link
Member

@dlorenc dlorenc commented Dec 27, 2021

Without this we can't properly re-run IndexKeys() on the log data because old minisign
keys were stripped to no longer contain the KeyID or Algorithm field.

Signed-off-by: Dan Lorenc lorenc.d@gmail.com

Summary

Ticket Link

Fixes

Release Note


@dlorenc dlorenc force-pushed the minisignfix2 branch 2 times, most recently from b01c7b5 to f1e3105 Compare December 27, 2021 17:15
if err != nil {
t.Errorf("%v: cannot open %v", tc.caseDesc, tc.inputFile)
}
t.Run(tc.caseDesc, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to add more test cases in the tests slice above

Copy link
Contributor

Choose a reason for hiding this comment

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

Or do we cover all added new functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reorganized the tests a bit and added one more case!

@dlorenc
Copy link
Member Author

dlorenc commented Dec 28, 2021

The coverage is now at 89% for that package, and 100% for the new code:

image

cover.out Outdated Show resolved Hide resolved
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

fix looks fine, not sure we want to merge cover.out file though

pkg/pki/minisign/minisign.go Outdated Show resolved Hide resolved
Without this we can't properly re-run IndexKeys() on the log data because old minisign
keys were stripped to no longer contain the KeyID or Algorithm field.

Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

lgtm

@dlorenc dlorenc merged commit 01ef8ff into sigstore:main Dec 28, 2021
@dlorenc dlorenc deleted the minisignfix2 branch December 28, 2021 19:55
@shibumi
Copy link
Contributor

shibumi commented Dec 28, 2021

@bobcallaway merging the cover.out file is not necessart if we add gocoveralls or other coverage tooling to our CI pipeline.
This should provide us with a percentage overview for each PR. I am just not sure if its possible to show the coverage in the PR directly.

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.

3 participants