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

Fix connectors null command #376

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RafalKorepta
Copy link
Contributor

dd34b33

Run test against unreleased version of Redpanda subcharts
Copy the same solution as it was introduced in TestGoHelmEquivalence test case.

Reference
redpanda-data/helm-charts@c84b13a

f06a531

Do not pass empty command into connectors container

When sasl authorization is disabled the connectors container command is set to
null which causes whole startup to fail due to the fact that ./launch.sh bash
script configures all properties of connectors by default.

K8S-475

Copy the same solution as it was introduced in `TestGoHelmEquivalence` test case.

Reference
redpanda-data/helm-charts@c84b13a
@RafalKorepta RafalKorepta force-pushed the rk/k8s-475/fix-connectors-command branch from f06a531 to cfc5742 Compare January 9, 2025 14:42
When sasl authorization is disabled the connectors container command is set to
null which causes whole startup to fail due to the fact that `./launch.sh` bash
script configures all properties of connectors by default.
@RafalKorepta RafalKorepta force-pushed the rk/k8s-475/fix-connectors-command branch from cfc5742 to b8c1c23 Compare January 9, 2025 14:44
@@ -182,6 +181,12 @@ func Deployment(dot *helmette.Dot) *appsv1.Deployment {
},
},
}

if !helmette.Empty(values.Deployment.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix doesn't make sense to me.

	Command []string `json:"command,omitempty" protobuf:"bytes,3,rep,name=command"`

From the golang (Kubernetes) perspective, not setting Command and explicitly setting Command to nil are equivalent. Is there something I'm overlooking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

transpiled template code is always setting command regardless of the command.

Go code will work. Template not. You can see the diff in the golden file.

Comment on lines -10678 to +10640
- command: null
env:
- env:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on the fix @RafalKorepta

@chrisseto here's a part of the diff to which Rafal is referring to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but when the K8s API JSON unmarshals (You've read sig YAML, it's just JSON)command: null into a Container struct, there is no distinction between Command not being specified and being explicit set to null/nil. The behavior is present for many other fields that show up as null (e.g. creationTimestamp).

Screen.Recording.2025-01-09.at.10.20.11.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then I will dig further why connectors does not work

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.

3 participants