Allow retention duration empty to default #954
Conversation
knative-prow-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eric-sap The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## main #954 +/- ##
=======================================
Coverage 75.64% 75.65%
=======================================
Files 137 137
Lines 6545 6547 +2
=======================================
+ Hits 4951 4953 +2
Misses 1311 1311
Partials 283 283
Continue to review full report at Codecov.
|
| // or to the explicit canonical zero, "P0D" (which is an empty Period struct). | ||
| // This particular change needs to be allowed, otherwise upgrading old channels will fail. | ||
| if original.Spec.RetentionDuration == "" { | ||
| if kc.Spec.RetentionDuration == constants.DefaultRetentionISO8601Duration || |
There was a problem hiding this comment.
should we release note this change?
There was a problem hiding this comment.
yeah, definitely - this was my fault - two successive changes to the same area - thanks eric.
|
I've simplified it to a simple empty string check and a large comment about the reasoning. If this is okay to merge the I will do that and then port it back to the release-0.26 branch. I do think this should be left in main as well, since despite it being "required" to run the post-install job after installing v0.26, this is inevitably going to be neglected by some users and it would be better if the empty field were still able to be set to a proper value in v0.27+ |
knative-prow-robot
added
size/L
|
The following is the coverage report on the affected files.
|
|
Thanks for fixing - sorry for the trouble! /lgtm |
* Allow the retentionDuration to be changed from empty to the default * Also allow empty string to P0D * imports * imports * imports * imports * Simplify to allow any change from empty-string * Don't allow other immutable changes just because retentionDuration is changing * style * Linter doesn't care for passing nil contexts * Skip the recursion and just alter the ignoreArguments * imports
* Allow the retentionDuration to be changed from empty to the default * Also allow empty string to P0D * imports * imports * imports * imports * Simplify to allow any change from empty-string * Don't allow other immutable changes just because retentionDuration is changing * style * Linter doesn't care for passing nil contexts * Skip the recursion and just alter the ignoreArguments * imports
…native-extensions#957) * Allow the retentionDuration to be changed from empty to the default * Also allow empty string to P0D * imports * imports * imports * imports * Simplify to allow any change from empty-string * Don't allow other immutable changes just because retentionDuration is changing * style * Linter doesn't care for passing nil contexts * Skip the recursion and just alter the ignoreArguments * imports
…native-extensions#957) (#413) * Allow the retentionDuration to be changed from empty to the default * Also allow empty string to P0D * imports * imports * imports * imports * Simplify to allow any change from empty-string * Don't allow other immutable changes just because retentionDuration is changing * style * Linter doesn't care for passing nil contexts * Skip the recursion and just alter the ignoreArguments * imports Co-authored-by: eric-sap <eric.van.heest@sap.com>
Fixes #952
Proposed Changes
Without this change, running the channel-post-install.yaml for eventing-kafka v0.26.3 can crash if there are kafkachannel resources from a previous version that don't have the RetentionDuration field in the spec.
I am sure that this change "fixes the issue" ... however, I am not sure that "allowing exceptions to immutability" is an okay thing to be doing (However, I also don't know any other way to fix this, so comments are very welcome please).
Release Note