Update CRD and fix minor issues #164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
../../../../root/go/pkg/mod/github.com/redpanda-data/helm-charts@v0.0.0-20240624182334-f539d8f02f09/charts/redpanda/helpers.go:311:8: cannot infer T (/root/go/pkg/mod/k8s.io/utils@v0.0.0-20240310230437-4693a0247e57/ptr/ptr.go:56:12) (typecheck)
--
| sc := ptr.Deref(values.Statefulset.PodSecurityContext, values.Statefulset.SecurityContext)
| ^
| ../../../../root/go/pkg/mod/github.com/redpanda-data/helm-charts@v0.0.0-20240624182334-f539d8f02f09/charts/redpanda/helpers.go:325:8: cannot infer T (/root/go/pkg/mod/k8s.io/utils@v0.0.0-20240310230437-4693a0247e57/ptr/ptr.go:56:12) (typecheck)
| sc := ptr.Deref(values.Statefulset.PodSecurityContext, values.Statefulset.SecurityContext)
| ^
| ../../../../root/go/pkg/mod/github.com/redpanda-data/helm-charts@v0.0.0-20240624182334-f539d8f02f09/charts/redpanda/service.loadbalancer.go:86:8: cannot infer T (/root/go/pkg/mod/k8s.io/utils@v0.0.0-20240310230437-4693a0247e57/ptr/ptr.go:56:12) (typecheck)
| if !ptr.Deref(listener.Enabled, values.External.Enabled) {
| ^
| ../../../../root/go/pkg/mod/github.com/redpanda-data/helm-charts@v0.0.0-20240624182334-f539d8f02f09/pkg/gotohelm/helmette/sprig.go:313:17: cannot range over n (variable of type int) (typecheck)
| for i := range n {
I feel like we should disable or update typecheck linter. It might be a problem that helm chart is using 1.22.4 https://github.com/redpanda-data/helm-charts/blob/981683cc95b7013673d3ea28c753cb048232f8ce/go.mod#L3 go version. redpanda-operator on the other hand is using 1.21
redpanda-operator/src/go/k8s/go.mod
Line 3 in ff0c0c1
Prior to this commit golangci-lint was installed by task and run in buildkite, installed by an action and run in github actions, and there existed two `.golangci.yml` files. This scattered configuration was frustrating to deal with and resulted in linting rules being applied inconsistently (aside from the golangci's own inconsistency). This commit collapses the configurations down to a single file in `src/go/k8s/.golangci.yml`, updates the configurations to fix some deprecations, removes the github action for linting in favor of builtkite, installed golangci-lint via nix, and fixes pre-existing errors discovered by fixing the configuration spaghetti.
This commit updates `RedpandaClusterSpec` to support the `TrustStore` option that was added to the helm chart in redpanda-data/helm-charts@f539d8f. Additionally, it adds a regression test to find divergences between the CRD and helm chart. The majority of this test case is currently disabled due to many such divergences. Finally, this commit also contains a handful of small fixes that were found in the above test case: - Added `omitempty` to `Auth.SASL`, `RedpandaMemory.{Memory,ReserveMemory}`. - Deprecated `ListenerTLS.SecretRef` as it's not a real value. - Deprecated `UsageStats.Organization` as it's been removed from the helm chart. - Removed unused types `Limits`, `Requests`, `TopologySpreadConstraintsItems`. - Corrected `TopologySpreadConstraint` to be a slice. (See note) Technically, changing the type of `TopologySpreadConstraint` is a breaking change. However, specifying it would have caused deployments to immediately break. Therefore it's reasonably safe to assume this change will not affect any users. Changing the casing of `FullNameOverride` is being deferred as it is possible to specify it without breaking deployments and changing the casing could result in breakages as that would be seen as a field addition and removal from the API perspective.
chrisseto
force-pushed
the
chris/p/crd-updates
branch
from
176d56e
to
81ff038
Compare
|
I fixed up out golangci-lint configuration in this commit e2560d7. Linting should be much more consistent now though you'll need to rely on nix develop and task to run it until we deal with the structure of the repo in more detail. |
RafalKorepta
approved these changes
Jun 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates
RedpandaClusterSpecto support theTrustStoreoption that was added to the helm chart inredpanda-data/helm-charts@f539d8f.
Additionally, it adds a regression test to find divergences between the CRD and helm chart. The majority of this test case is currently disabled due to many such divergences.
Finally, this commit also contains a handful of small fixes that were found in the above test case:
omitemptytoAuth.SASL,RedpandaMemory.{Memory,ReserveMemory}.ListenerTLS.SecretRefas it's not a real value.UsageStats.Organizationas it's been removed from the helm chart.Limits,Requests,TopologySpreadConstraintsItems.TopologySpreadConstraintto be a slice. (See note)Technically, changing the type of
TopologySpreadConstraintis a breaking change. However, specifying it would have caused deployments to immediately break. Therefore it's reasonably safe to assume this change will not affect any users.Changing the casing of
FullNameOverrideis being deferred as it is possible to specify it without breaking deployments and changing the casing could result in breakages as that would be seen as a field addition and removal from the API perspective.