✨ reduce use of carvel validators #1269
Conversation
by implementing our own. This gives us more freedom in maing changes and fixing bugs in the actual validations instead of having to push all fixes up to the carvel/kapp project. Signed-off-by: everettraven <[email protected]>
✅ 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 #1269 +/- ##
==========================================
+ Coverage 76.51% 77.87% +1.36%
==========================================
Files 40 40
Lines 2363 2509 +146
==========================================
+ Hits 1808 1954 +146
Misses 389 389
Partials 166 166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -32,17 +32,18 @@ type Preflight struct { | |||
|
|
|||
| func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface, opts ...Option) *Preflight { | |||
| changeValidations := []kappcus.ChangeValidation{ | |||
There was a problem hiding this comment.
How much further would we need to go to fully drop the kapp dependency?
There was a problem hiding this comment.
I think it would mostly be anything in our codebase that uses the kappcus import library which is probably just in this package. Level of effort depends on whether we want to re-implement in our own way such that we don't need to adhere to Apache 2.0 license guidelines or if we would want to copy-paste the code we need including maintaining and adhering to the Apache 2.0 license guidelines
There was a problem hiding this comment.
That being said, I would say totally dropping the kapp dependency should be seen outside the scope of this PR. While this PR reduces our use of the carvel provided validators, the main reason for this PR is to address some minor bugs in the output when the checks fail. My approach to doing this just happens to reduce the carvel validator use because I felt it would be easier for us to maintain long-term as opposed to contributing the changes all the way upstream.
There was a problem hiding this comment.
Are the check implementations copy/pasted from kapp or new implementations? I ask because:
- If they are copy/pasted:
- I don't need to review them as carefully,
- BUT, we need some sort of attribution?
- If they are not copy/pasted,
- We should review thoroughly
- We don't need attribution
There was a problem hiding this comment.
They are not copy/paste-ed but inspired by the carvel implementation (which I helped write so hard to not be inspired by), but I think the implementation is changed significantly enough that it doesn't require any attribution
Signed-off-by: everettraven <[email protected]>
|
Where are we on this one? |
Description
Reduces the use of the field change validators in carvel/kapp in favor of our own implementations. This makes the CRD Upgrade Safety pre-flight check more maintainable by allowing us to make bugfixes and changes in our implementation without having to contribute to carvel/kapp and wait for a new release.
fixes [CRD Upgrade Safety] Message output use the human readable value instead of exact bytes #1248
fixes [CRD Upgrade Safety] Add a change validation for handling field type changes #1246
Reviewer Checklist