✨ Add field to Extension API to skip crd safety checks #699
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
=======================================
Coverage 64.01% 64.01%
=======================================
Files 22 22
Lines 1370 1370
=======================================
Hits 877 877
Misses 442 442
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| //+kubebuilder:Optional | ||
| // | ||
| // skipCRDUpgradeSafetyCheck specifies whether or not the CRD upgrade safety checks should be skipped when attempting to install the extension | ||
| SkipCRDUpgradeSafetyCheck bool `json:"skipCRDUpgradeSafetyCheck,omitempty"` |
There was a problem hiding this comment.
I think in the past we identified that bool fields are problematic over time since quite frequently an additional value would slip in, so for maximum flexibility we've been using enum/string for this role, like so.
Is there any reason not to do so here?
There was a problem hiding this comment.
I don't disagree, but I recall us having also gone back and forth on our feelings on that. I'm happy to make a change to this, but I do think we should make sure we come up with a consistent approach as a project and that we (maintainers) all agree to prevent flip-flopping.
There was a problem hiding this comment.
IMO, let's go ahead and get this merged to unblock further development. We'll separately plan for a full API review once we have all of the MVP functionality complete, and do any necessary refactoring at that point.
|
I'm surprised I don't see one of:
|
That's a good point, I totally forgot to run |
|
It's |
Signed-off-by: everettraven <everettraven@gmail.com>
everettraven
dismissed stale reviews from joelanford and perdasilva
via
66e614a
everettraven
force-pushed
the
feature/crd-upgrade-safety-field
branch
from
0b56e4f
to
66e614a
Compare
|
@joelanford I just pushed some updates to fix what you pointed out was missing. Great catch! |
Description
Adds an optional
Extension.spec.skipCRDUpgradeSafetyChecksfield that when set totruewill be used to disable the CRD upgrade safety checks when attempting to install an extension.No logic included in this PR as the CRD Upgrade Safety preflight check hasn't been fully implemented in carvel-dev/kapp yet.
I figured it doesn't hurt to go ahead and add the field and have it do nothing for now, but I'm also happy for this PR to be put on hold until the underlying logic to enable/disable the check is able to be put in place.
fixes #658
Reviewer Checklist