-
Notifications
You must be signed in to change notification settings - Fork 10
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
Set explicitly entry point for configurator #368
base: main
Are you sure you want to change the base?
Conversation
5cc8003
to
c92dff4
Compare
0cc6c13
to
6cbd7c3
Compare
@@ -366,8 +357,6 @@ func operatorArguments(dot *helmette.Dot) []string { | |||
"--health-probe-bind-address=:8081", | |||
"--metrics-bind-address=127.0.0.1:8080", | |||
"--leader-elect", | |||
fmt.Sprintf("--configurator-tag=%s", configuratorTag(dot)), |
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.
Should the corresponding values be removed or marked as deprecated as they're now a no-op?
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 can mark it as deprecated. My intent was to remove argument form deployment arguments to not confuse end user.
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.
After more thought both option should stay as is. Any user could potentially want to overwrite the "configurator" container registry and it's tag. It shouldn't be encourage, but some K8S cluster can be cut from the internet. Internal container registry would be then used and user is free to specify helm chart additionalCmdFlags
to pass configurator-tag
and configurator-base-image
values
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.
but some K8S cluster can be cut from the internet
Great call out!
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.
Does that mean you'll be reverting the change this comment thread is on?
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.
Nope I want to leave this unspecified by default and allow for overwrite with additionalCmdFlags if particular user is air gapped.
99e0ed2
to
1f934fb
Compare
Command: []string{"/redpanda-operator"}, | ||
Args: []string{"configure"}, |
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.
So, since we're modifying the entrypoint of the configurator -- I believe (assuming we also use it in v2) we'll need to mirror this change in the Redpanda helm chart as well (do we want to do that before or after the rsync work in the helm-charts repo?).
That said, I think this warrants a callout in a changelog, specifically stating that if you're referencing a custom configurator image you'll need to:
- Leverage the additionalCmdFlags to override the configurator image as the other flag is now deprecated
- Ensure that the mirrored container supports this entrypoint (maybe just say that the configurator image must be based on vX.Y.Z of the operator image?)
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 believe (assuming we also use it in v2) we'll need to mirror this change in the Redpanda helm chart as well
We don't use this exact configurator between v1 and v2.
That said, I think this warrants a callout in a changelog
I will add change log statement about configurator entrypoint.
1f934fb
to
e670cf7
Compare
On top of #367
From 27/08/2024 the operator container can be used as configurator. With this change any user of the Cluster custom resource can now use single container without specifing arguments for configurator.
Reference
a897ad3 #211 0868b61 #322