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

Add e2e test for attest / verify-attestation #1685

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Mar 30, 2022

Signed-off-by: Ville Aikas vaikas@chainguard.dev

Summary

Add an e2e test for validating creation of
an attestation and then validating it against a cue policy that will work and one that
doesn't work. Now, This is the example that we use in our documentation, and it does
now pass. I reckon with these tests we can add few more of these test cases with
different payload types and keep validating them by adding them in the
test/testdata/policies. Just a beginning.

Ticket Link

Fixes

Release Note

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2022

Codecov Report

Merging #1685 (42f97af) into main (ba50ee0) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1685      +/-   ##
==========================================
+ Coverage   29.29%   29.33%   +0.04%     
==========================================
  Files         140      140              
  Lines        8370     8358      -12     
==========================================
  Hits         2452     2452              
+ Misses       5652     5640      -12     
  Partials      266      266              
Impacted Files Coverage Δ
cmd/cosign/cli/verify/verify_attestation.go 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba50ee0...42f97af. Read the comment docs.

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

that is cool, thanks

// we need to check only given type from the cli flag
// so we are skipping other types
if predicateURI != val {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing this check altogether causes the --type parameter to be completely ignored and the policy to be applied to all attestations found. My reading of the code is that this is not intended. I tried a different take in resolving this issue in the first commit of #1672.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds great :) I was just pulling my hair out and wanted to get a working set. My proposal is to get your #1672 @lcarva in, and then I can add the e2e tests only as a follow up to that, and then maybe we can work together on adding some e2e tests for the rego stuff that you're working on? What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good plan :)

I think I finally fixed up the failing test on Windows (issue on rego itself). The test needs to be manually triggered because I'm a first-time contributor to this repo.

a cue policy both with success and failure.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas vaikas changed the title Remove the non-functioning sanity check, add e2e test Add e2e test for attest / verify-attestation Mar 30, 2022
@dlorenc dlorenc merged commit 9496416 into sigstore:main Mar 30, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 30, 2022
@vaikas vaikas deleted the cosigned-attestations branch March 30, 2022 16:31
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* Remove the non-functioning sanity check, add e2e test for validating
a cue policy both with success and failure.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* oops, forgot to use test env variables instead of local tests :)

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
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.

None yet

5 participants