-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for Secret or ConfigMap based environment variables in ContainerTemplate
#10623
Add support for Secret or ConfigMap based environment variables in ContainerTemplate
#10623
Conversation
CotnainerTemplate
ContainerTemplate
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
…tnainerTemplate Signed-off-by: Jakub Scholz <[email protected]>
c5cbfb2
to
df74a2d
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.
Looks good. Few minor suggestions.
Assumed that we weren't including config in the MM2 example intentionally
api/src/main/java/io/strimzi/api/kafka/model/common/ContainerEnvVar.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/connect/ExternalConfiguration.java
Outdated
Show resolved
Hide resolved
documentation/modules/configuring/con-config-kafka-connect.adoc
Outdated
Show resolved
Hide resolved
documentation/modules/configuring/con-configuration-points-configmaps.adoc
Outdated
Show resolved
Hide resolved
Signed-off-by: Jakub Scholz <[email protected]>
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.
Wording nit, but otherwise LGTM
@@ -49,6 +52,15 @@ public void setValue(String value) { | |||
this.value = value; | |||
} | |||
|
|||
@Description("Reference to secret or config map property to which will be the environment variable is set.") |
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 see Paul reviewed this before, but it still isn't quite what he proposed and didn't make sense to me, so re-proposing what he suggested.
@Description("Reference to secret or config map property to which will be the environment variable is set.") | |
@Description("Reference to the secret or config map property to which the environment variable is set.") |
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.
Fixed in #10628
@scholzj I was too slow with my review! |
@katheris I will open a separate Pr to fix it. |
Type of change
Description
This PR implements the Strimzi Proposal #85 and adds the possibily to configure Secret or ConfigMap based environment variables in any container through the container template section.
It also deprecates the rest of the
externalConfiguration
section in Connect and MM2 that allowed the same, but only in the Connect and MM2 resources.Checklist