-
Notifications
You must be signed in to change notification settings - Fork 64
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 type to policy CRD #1079
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1079 +/- ##
==========================================
- Coverage 58.31% 52.32% -5.99%
==========================================
Files 93 101 +8
Lines 5546 6291 +745
==========================================
+ Hits 3234 3292 +58
- Misses 1996 2682 +686
- Partials 316 317 +1
☔ View full report in Codecov by Sentry. |
8b2a90d
to
55b774d
Compare
55b774d
to
9731e9c
Compare
i would usually think with PRs introducing new properties, there should be a version update. However customer probably doesn't have any custom policy. Any Policy CR from RC8, would be automatically updated with a CR with the type property. Is that correct? Also interested to hear feedback from @akashsinghal |
good call. I'm open to any advice on version update, I'll wait for more feedback on it as it's not an urgent PR. |
I think the change not purely additive and is a breaking change if I'm understanding correctly? Ther user will need to update the CR to provide the type of policy in the I think the most thorough way would be to bump the CR version and add the conversion logic between the versions. Now, if we think that disruption will be minimal due to the fact that the original CR was originally introduced in an RC version, then might be ok to not bump. |
5f34d9a
to
c6398a0
Compare
db93e6e
to
54d16bf
Compare
837c0f1
to
d7f5ef9
Compare
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. One question: what is the upgrade process like again for the new CRD version? If a user has v1alpha1 already installed, will the next pre-install hook in the new helm chart automatically upgrade. Or does user need to manually delete the old CRD def for policy before the new install?
d7f5ef9
to
97b48eb
Compare
CRD would be auto-installed, but users need to apply new CRs in v1beta1 to make it work. |
d2e086e
to
bc44880
Compare
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, thanks Binbin
Signed-off-by: Binbin Li <[email protected]>
Signed-off-by: Binbin Li <[email protected]>
Description
What this PR does / why we need it:
metadata.name
to beratify-policy
only.spec.type
to berego-policy
orconfig-policy
only.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #1067
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Checklist:
Post Merge Requirements
Helm Chart Change