-
Notifications
You must be signed in to change notification settings - Fork 53
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 external data provider #134
add external data provider #134
Conversation
Signed-off-by: Sertac Ozercan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #134 +/- ##
==========================================
- Coverage 42.28% 41.51% -0.78%
==========================================
Files 46 52 +6
Lines 2980 3122 +142
==========================================
+ Hits 1260 1296 +36
- Misses 1329 1435 +106
Partials 391 391
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Sertac Ozercan <[email protected]>
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.
Awesome! Mostly clear cut comments. The thorniest comments are around the Rego function signature and whether we should have a more well-defined wire format.
constraint/config/crds/externaldata.gatekeeper.sh_providers.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
e551920
to
e35c157
Compare
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
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.
Just a few questions, nothing major I don't think.
constraint/config/crds/externaldata.gatekeeper.sh_providers.yaml
Outdated
Show resolved
Hide resolved
spec: | ||
description: ProviderSpec defines the desired state of Provider | ||
properties: | ||
maxRetry: |
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.
Maybe we should put max retries as part of the invocation of the provider? I could see validation more retry-tolerant than mutation, for example.
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.
you mean as part of external_data()
(only for validation so remove maxretry
from provider)? do we want to set up a default value if user doesn't provide this (1?)?
Signed-off-by: Sertac Ozercan <[email protected]>
constraint/config/crds/externaldata.gatekeeper.sh_providers.yaml
Outdated
Show resolved
Hide resolved
constraint/config/crds/externaldata.gatekeeper.sh_providers.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
@maxsmythe any unresolved comments? |
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.
Couple small things, but ready to merge after that.
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
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.
LGTM!
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Rita Zhang [email protected]
Signed-off-by: Sertac Ozercan [email protected]
Design doc: https://docs.google.com/document/d/1hPi86jdsCKg8puYT5_s_73mPGExUJeZfyKmvG-XWtPc/edit