-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat: add requiresSyncData and verify-sync test #251
Conversation
I left the template versions as-is given the actual rego/etc hasn't changed, but let me know if we should bump the minor or patch. Thanks! |
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.
Basically LGTM. Ideally we'd use more reliable detection for data.inventory
usage, but I think requiring it would depend on how much work that entails.
The question around "what about clusters for whom these resources don't exist" is salient if we want to think about the expected uses of these annotations.
Also, WRT template versions, leaving them as-is seems right to me (no Rego changes) |
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: Andrew Peabody <andrewpeabody@google.com>
artifacthub/library/general/poddisruptionbudget/1.0.0/template.yaml
Outdated
Show resolved
Hide resolved
Pending #255 |
Signed-off-by: Julian Katz <juliankatz@google.com>
Signed-off-by: Andrew Peabody <andrewpeabody@google.com>
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.
1 nit, but LGTM
Signed-off-by: Andrew Peabody <andrewpeabody@google.com>
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 after 1 nit
Signed-off-by: Andrew Peabody <andrewpeabody@google.com>
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.
minor comment, otherwise LGTM
Signed-off-by: Andrew Peabody <andrewpeabody@google.com>
Signed-off-by: Andrew Peabody <andrewpeabody@google.com>
Hi Everyone - Is anything else needed before this can be merged? Thanks! |
@ritazh any outstanding 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.
LGTM
metadata.gatekeeper.sh/requiresSyncData
annotation to templates usingdata.inventory
sync.yaml
foruniqueingresshost
templaterequire-sync
test which requires templates using data.inventory have both async.yaml
andmetadata.gatekeeper.sh/requiresSyncData
annotationFixes #248