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

Remove direct storage.Path requirement in handler #208

Merged
merged 8 commits into from
Apr 11, 2022

Conversation

willbeason
Copy link
Member

As-is requires a direct dependency of all callers on OPA. This PR
changes the TargetHandler interface to accept storage path as a slice of
strings instead of storage.Path.

Functionally this changes nothing.

Signed-off-by: Will Beason willbeason@google.com

As-is requires a direct dependency of all callers on OPA. This PR
changes the TargetHandler interface to accept storage path as a slice of
strings instead of storage.Path.

Functionally this changes nothing.

Signed-off-by: Will Beason <willbeason@google.com>
Will Beason added 2 commits April 4, 2022 08:58
Signed-off-by: Will Beason <willbeason@google.com>
The workflow seems broken? Upgrading to fix.

Signed-off-by: Will Beason <willbeason@google.com>
@willbeason
Copy link
Member Author

Gatekeeper test failing is expected since this changes the Target interface.

@willbeason willbeason self-assigned this Apr 4, 2022
Signed-off-by: Will Beason <willbeason@google.com>
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #208 (2fda96a) into master (7b86d0c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #208   +/-   ##
=======================================
  Coverage   46.29%   46.29%           
=======================================
  Files          64       64           
  Lines        2819     2819           
=======================================
  Hits         1305     1305           
  Misses       1297     1297           
  Partials      217      217           
Flag Coverage Δ
unittests 46.29% <ø> (ø)

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


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 7b86d0c...2fda96a. Read the comment docs.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

Signed-off-by: Will Beason <willbeason@google.com>
Will Beason added 2 commits April 7, 2022 07:03
We've been at the threshold of about 5 minutes for a while, causing
flaky tests. We recently enabled --race, which slows all tests and is
likely what makes the tests run too slow in GitHub actions to run within
the current 5 minute timeout.

Note that in order to run, this target must create an entirely new
Gatekeeper docker image with every invocation - Docker does not cache
steps which involve file operations, so the image must be built from the
ground up each time. This alone takes up a couple minutes of the
runtime.

As stated in the PR discussion, I see no performance degradation in the
frameworks tests. I've had trouble getting Gatekeeper tests to pass at
all from frameworks in GitHub, but every run I've done locally has
worked.

Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason merged commit 90403ea into open-policy-agent:master Apr 11, 2022
@willbeason willbeason deleted the remove-directo-opadep branch April 11, 2022 19:47
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

4 participants