-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix for tiered storage enable flag #136
Conversation
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 but we need to have some unit test on the conversions before merging. It's way to easy for a typo to slip in unnoticed in conversion code.
dst.Spec.Migration = src.Spec.Migration | ||
dst.Spec.ChartRef = src.Spec.ChartRef | ||
|
||
{ |
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.
I'm a bit confused about why we're creating a new scope here?
I think you could remove a lot of this code and instead marshal src
to JSON and the unmarshal it into dst
? You'll need to remove the incorrectly typed boolean before and after but everything else should "just work".
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.
Ok I will try to marshal and unmarshal it.
func (*Redpanda) Hub() {} | ||
|
||
// SetupWebhookWithManager will setup the manager to manage the webhooks | ||
func (r *Redpanda) SetupWebhookWithManager(mgr ctrl.Manager) error { |
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.
I know the setup with manager is a common convention but it feels very strange to have this implemented on the CRD itself. Is doing so a common conversion for conversion webhooks?
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.
Yes, it would be generated by kubebuilder and I just follow it.
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.
2f235e2
to
7aac823
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.
Lots of nits. Only blocker is that I strongly prefer to not be using a string replace within JSON marshaling.
return fmt.Errorf("marshaling v1alpha1.Redpanda: %w", err) | ||
} | ||
|
||
r := string(b) |
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.
This feels pretty sketchy to me. Why not do something like this:
if err := json.Unmarshal(b, dst); err != nil {
return err
}
if src.Spec.ClusterSpec != nil && ... {
dst.Spec.ClusterSpec.Storage.Tiered.Config.CloudStorageEnabled = src.Spec.ClusterSpec.CloudStorageEnabled == "true"
}
Alternatively, you could make a custom boolean type for CloudStorageEnabled
that supports unmarshalling a string into a boolean value and then you could avoid all the != nil
checks as they'd be handled for you.
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.
This feels pretty sketchy to me. Why not do something like this:
I don't understand why empty object (a destination named dst
) should be unmarshaled? The data is located in the source src
.
The Redpanda CRD version `v1alpha1` was working up until Redpanda helm chart defined `cloud_storage_enabled` as boolean in values schema. From the `5.8.0` release the schema change is publicly available. Affected commit redpanda-data/helm-charts@b733738 Reference https://github.com/redpanda-data/helm-charts/releases/tag/redpanda-5.8.0
8eb4180
to
baf90a4
Compare
Due to a repetition for Redpanda in version v1alpha1 and v1alpha2 the kubectl apply fails with ``` The CustomResourceDefinition "redpandas.cluster.redpanda.com" is invalid: metadata.annotations: Too long: must have at most 262144 bytes ``` With server-side flag the annotation is not added to each resource. Reference https://github.com/kubernetes/kubectl/blob/514f46729f82412dd9cc41f206058bc4ae9b62b0/pkg/cmd/apply/apply.go#L762-L784
baf90a4
to
b0d4bf8
Compare
Create v1alpha2 version of the Redpanda CRD
The Redpanda CRD version
v1alpha1
was working up until Redpanda helm chartdefined
cloud_storage_enabled
as boolean in values schema. From the5.8.0
release the schema change is publicly available.
Affected commit redpanda-data/helm-charts@b733738
New v1alpha2 version is set as storage version. All conversion is implemented in Redpanda version v1alpha1.
Reference
https://github.com/redpanda-data/helm-charts/releases/tag/redpanda-5.8.0