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

2476 predicate type download #2484

Merged
merged 12 commits into from
Feb 8, 2023

Conversation

chaospuppy
Copy link
Contributor

Relates to #2476

Summary

This PR will add a predicateType annotation to the manifest of each attestation. This annotation can then be used with the cosign download attestation command to output only the desired annotation, using a new —predicate-type flag. If the flag is used, but no predicateType annotation is matched, an error is raised. If the flag is not provided, all attestations are returned, as they are currently.

Release Note

New Feature: filter attestations output from cosign download attestation command by using the —predicate-type flag.

Documentation

This will require an update to the README for the cosign download command, which will be updated provided this seems like a good idea.

@chaospuppy chaospuppy force-pushed the 2476-predicate-type-download branch 2 times, most recently from 9a1f267 to 75b780b Compare November 27, 2022 16:34
pkg/cosign/fetch.go Outdated Show resolved Hide resolved
@chaospuppy
Copy link
Contributor Author

Resolved all threads @hectorj2f

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

Regardless of the proposed solution, we are missing tests here.

@chaospuppy
Copy link
Contributor Author

Looks like I have a few more places to set arguments to FetchAttestationsForReference as well.

@codecov-commenter
Copy link

Codecov Report

Merging #2484 (0d76a22) into main (72dac4d) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2484      +/-   ##
==========================================
- Coverage   30.03%   29.95%   -0.09%     
==========================================
  Files         139      139              
  Lines        8586     8609      +23     
==========================================
  Hits         2579     2579              
- Misses       5649     5672      +23     
  Partials      358      358              
Impacted Files Coverage Δ
cmd/cosign/cli/download.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/download.go 0.00% <0.00%> (ø)
pkg/cosign/fetch.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chaospuppy
Copy link
Contributor Author

E2E tests have been updated, as has the help text for --predicate-type.

@chaospuppy
Copy link
Contributor Author

@hectorj2f any outstanding issues on this PR that you see?

@chaospuppy
Copy link
Contributor Author

Hi again @hectorj2f, just pinging on this again to see if it could be merged in.

// AddFlags implements Interface
func (o *AttestationDownloadOptions) AddFlags(cmd *cobra.Command) {
cmd.Flags().StringVar(&o.PredicateType, "predicate-type", "",
"download attestation with matching predicateType annotation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Future suggestion: this could be an array of matching annotations.

@hectorj2f
Copy link
Contributor

I added more reviewers to avoid missing anything here. It looks good to me.

znewman01
znewman01 previously approved these changes Feb 6, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Seems ok to me

@znewman01
Copy link
Contributor

BTW you may have to rebase to get tests to run, sorry! I'll re-approve quick this time.

Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
…e-type option for attestation download

Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
…p text for --predicate-type

Signed-off-by: Tim Seagren <seagren.tim@solute.us>
Signed-off-by: Tim Seagren <seagren.tim@solute.us>
@chaospuppy
Copy link
Contributor Author

@znewman01 no worries, rebased.

@znewman01 znewman01 merged commit dd747ee into sigstore:main Feb 8, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Feb 8, 2023
dmitris pushed a commit to dmitris/cosign that referenced this pull request Mar 24, 2023
* add cyclonedxxml predicatetype

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* read cyclonedx xml bytes into cyclonedx.SBOM

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* add e2e test

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* update cli docs

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* add artibrary annotations to mainfest

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* add predicateType annotation to attestation manifests, add --predicate-type option for attestation download

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* remove unrelated changes

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* update PredicateType as field of c

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* apply suggestions

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* remove unused index var

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* add e2e tests for attestation download, resolve data race, update help text for --predicate-type

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

* CreateAttestationReplace

Signed-off-by: Tim Seagren <seagren.tim@solute.us>

---------

Signed-off-by: Tim Seagren <seagren.tim@solute.us>
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.

4 participants