Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

Allow retention duration empty to default #954

Merged
merged 12 commits into from
Oct 27, 2021
Merged

Allow retention duration empty to default #954

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eee0015
Allow the retentionDuration to be changed from empty to the default
eric-sap Oct 25, 2021
771dfcd
Also allow empty string to P0D
eric-sap Oct 25, 2021
bbd85a6
imports
eric-sap Oct 26, 2021
15e64e3
imports
eric-sap Oct 26, 2021
b3173f8
imports
eric-sap Oct 27, 2021
b891c46
imports
eric-sap Oct 27, 2021
90e7855
Simplify to allow any change from empty-string
eric-sap Oct 27, 2021
70a2fe4
Don't allow other immutable changes just because retentionDuration is…
eric-sap Oct 27, 2021
b77eaa7
style
eric-sap Oct 27, 2021
01625a4
Linter doesn't care for passing nil contexts
eric-sap Oct 27, 2021
a739b43
Skip the recursion and just alter the ignoreArguments
eric-sap Oct 27, 2021
827d657
imports
eric-sap Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/apis/messaging/v1beta1/kafka_channel_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ import (
"fmt"

"github.com/google/go-cmp/cmp/cmpopts"
"github.com/rickb777/date/period"
"knative.dev/eventing/pkg/apis/eventing"
"knative.dev/pkg/apis"
"knative.dev/pkg/kmp"

"knative.dev/eventing-kafka/pkg/common/constants"
)

func (kc *KafkaChannel) Validate(ctx context.Context) *apis.FieldError {
Expand Down Expand Up @@ -90,6 +93,15 @@ func (kc *KafkaChannel) CheckImmutableFields(_ context.Context, original *KafkaC
Details: err.Error(),
}
} else if diff != "" {
// Check the specific case of RetentionDuration being "updated" from the empty string to the default ("PT168H")
// 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 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we release note this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, definitely - this was my fault - two successive changes to the same area - thanks eric.

kc.Spec.RetentionDuration == period.Period.String(period.Period{}) {
return nil
}
}
return &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Paths: []string{"spec"},
Expand Down
80 changes: 80 additions & 0 deletions pkg/apis/messaging/v1beta1/kafka_channel_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import (
"knative.dev/eventing/pkg/apis/eventing"
"knative.dev/pkg/apis"
"knative.dev/pkg/webhook/resourcesemantics"

"knative.dev/eventing-kafka/pkg/common/constants"
)

func TestKafkaChannelValidation(t *testing.T) {
Expand Down Expand Up @@ -328,6 +330,84 @@ func TestKafkaChannelImmutability(t *testing.T) {
}
}(),
},
"updating immutable retentionDuration (empty to default, exception to rule)": {
original: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "",
},
},
updated: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: constants.DefaultRetentionISO8601Duration,
},
},
},
"updating immutable retentionDuration (empty to canonical zero P0D, exception to rule)": {
original: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "",
},
},
updated: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "P0D",
},
},
},
"updating immutable retentionDuration (non-empty to default)": {
original: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "PT1H",
},
},
updated: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: constants.DefaultRetentionISO8601Duration,
},
},
want: func() *apis.FieldError {
return &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Paths: []string{"spec"},
Details: "{v1beta1.KafkaChannelSpec}.RetentionDuration:\n\t-: \"PT1H\"\n\t+: \"PT168H\"\n",
}
}(),
},
"updating immutable retentionDuration (empty to non-default)": {
original: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "",
},
},
updated: &KafkaChannel{
Spec: KafkaChannelSpec{
NumPartitions: 1,
ReplicationFactor: 1,
RetentionDuration: "PT100H",
},
},
want: func() *apis.FieldError {
return &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Paths: []string{"spec"},
Details: "{v1beta1.KafkaChannelSpec}.RetentionDuration:\n\t-: \"\"\n\t+: \"PT100H\"\n",
}
}(),
},
}

for n, test := range testCases {
Expand Down