-
Notifications
You must be signed in to change notification settings - Fork 556
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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.
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 |
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.
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.
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.
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?
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.
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>
42f97af
to
ea2537d
Compare
* 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>
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