Skip to content
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

v1alpha2: correct boolean unmarshalling #141

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

chrisseto
Copy link
Contributor

Previously, the specialized boolean types for cloud_storage_enabled, meant to ease the transition from v1alpha1 to v1alpha2, would silently fail in certain cases leading to the value of cloud_storage_enabled being dropped and set to nil.

This commit teaches both implementations how to unmarshal from both strings and booleans and correctly returns an error in cases where unmarshaling failed.

Users that upgraded to v2.1.19-24.1.2 will likely need to set cloud_storage_enabled as the conversion webhook will have removed whatever value was previously set.

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for the fix!

@RafalKorepta
Copy link
Contributor

I only changed/moved comments that addressed golangci-lint issues.

Previously, the specialized boolean types for `cloud_storage_enabled`,
meant to ease the transition from v1alpha1 to v1alpha2, would silently
fail in certain cases leading to the value of `cloud_storage_enabled`
being dropped and set to `nil`.

This commit teaches both implementations how to unmarshal from both
strings and booleans and correctly returns an error in cases where
unmarshaling failed.

Users that upgraded to `v2.1.19-24.1.2` will likely need to set
`cloud_storage_enabled` as the conversion webhook will have removed
whatever value was previously set.
The decommission-on-delete test case has been flaking quite frequently.
The team does not currently have time to investigate and fix the test.
For now, we're moving it to e2e-unstable to prevent it from holding up
releases and merges.

Fixing the test is tracked by #142.
@chrisseto chrisseto enabled auto-merge (rebase) May 17, 2024 14:05
@chrisseto chrisseto merged commit c655eb2 into main May 17, 2024
4 of 5 checks passed
@RafalKorepta RafalKorepta deleted the chris/fix-v1alpha2-repr branch May 23, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants