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 ec.fetch_oci_blob rego function #1107

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Conversation

lcarva
Copy link
Member

@lcarva lcarva commented Oct 18, 2023

It implements #1070, but it doesn't resolve it just yet. As a followup PR, I'll add acceptance tests to complete this functionality.

UPDATE: Acceptance tests are now included.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #1107 (0530d24) into main (976befd) will increase coverage by 0.08%.
Report is 23 commits behind head on main.
The diff coverage is 89.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
+ Coverage   84.16%   84.24%   +0.08%     
==========================================
  Files          63       64       +1     
  Lines        4988     5071      +83     
==========================================
+ Hits         4198     4272      +74     
- Misses        790      799       +9     
Flag Coverage Δ
acceptance 69.63% <73.07%> (+0.06%) ⬆️
generative 4.50% <ø> (ø)
integration 18.91% <ø> (ø)
unit 76.86% <89.74%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
internal/fetchers/oci/client.go 73.17% <75.00%> (+0.44%) ⬆️
internal/evaluator/rego.go 91.42% <91.42%> (ø)

... and 1 file with indirect coverage changes

@lcarva lcarva force-pushed the EC-130 branch 3 times, most recently from 1bbb320 to b54edef Compare October 18, 2023 19:03
Copy link
Member

@zregvart zregvart left a comment

Choose a reason for hiding this comment

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

I was wondering about the naming, and if ec.oci.blob could be a better name, or even if we want to push this upstream to OPA and have it opa.blob?
Some nitpicks inside, other than that 👍

internal/evaluator/rego.go Show resolved Hide resolved
internal/evaluator/rego.go Outdated Show resolved Hide resolved
internal/evaluator/rego.go Outdated Show resolved Hide resolved
internal/evaluator/rego.go Show resolved Hide resolved
@lcarva
Copy link
Member Author

lcarva commented Oct 19, 2023

I was wondering about the naming, and if ec.oci.blob could be a better name, or even if we want to push this upstream to OPA and have it opa.blob?

Updated to go with ec.oci.blob.

I think you probably meant oci.blob on your second suggestion. If so, that sounds like a good upstream name if/when this function is part of opa itself. For now, since it is implemented in EC, let's make it super obvious that it is related to EC by having the ec. prefix.

@lcarva lcarva marked this pull request as draft October 20, 2023 13:25
@lcarva
Copy link
Member Author

lcarva commented Oct 20, 2023

Caching is not working. I need to rethink the approach. Put it in draft for now.

@lcarva
Copy link
Member Author

lcarva commented Oct 23, 2023

I'm gonna hold off on the cache approach for this PR due to google/go-containerregistry#1821.

@lcarva lcarva marked this pull request as ready for review October 24, 2023 15:15
@lcarva
Copy link
Member Author

lcarva commented Oct 24, 2023

FYI, code coverage is failing to be uploaded. It is a known issue and it happens from time to time...

Resolves enterprise-contract#1070

Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
@lcarva lcarva merged commit 132d5c8 into enterprise-contract:main Oct 25, 2023
11 checks passed
@lcarva lcarva deleted the EC-130 branch October 25, 2023 15:20
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