-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: allow configuration of connect via ENV variables #44
Conversation
@@ -1,4 +0,0 @@ | |||
{% set kr_props = env_to_props('KSQL_', '') -%} |
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.
instead we are directly relying on https://github.com/confluentinc/ksql/blob/master/config/ksqldb-server.properties.template
|
||
# Default to 8083, which matches the mesos-overrides. This is here in case we extend the containers to remove the mesos overrides. | ||
if [ -z "${KSQL_CONNECT_REST_PORT:=}" ]; then | ||
export KSQL_CONNECT_REST_PORT=8083 |
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 think we need to change the run
script to source /etc/confluent/docker/configure
. I still don't think these exports in this file would be picked up when launching the server since configure
is exporting these variables in a sub-shell of the run
script. If we source the configure file in the run
script, the shell that's executing run
should pick up the env variables properly
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.
The change I made for the log4j env variable was moving the export from configure
to launch
So either these two exports in the file need to be moved to launch
or add modify run to source /etc/confluent/docker/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.
the change doesn't need to be picked up in the launch (unlike the change for LOG4J). The environemnt variable only needs to be present for the dub
command below in the configure
script
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 tested this to make sure that it works with docker-compose file without the env variable 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.
Oh I see, I thought it needed to be present during launch
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
|
||
# Default to 8083, which matches the mesos-overrides. This is here in case we extend the containers to remove the mesos overrides. | ||
if [ -z "${KSQL_CONNECT_REST_PORT:=}" ]; then | ||
export KSQL_CONNECT_REST_PORT=8083 |
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.
Oh I see, I thought it needed to be present during launch
A few changes to fix https://confluentinc.atlassian.net/browse/KSQL-4437
This change has a few small changes:
ksqldb-etc
instead of the ones that were in here (and deleteksql-server.properties.template
from hereThis change is inspired from: confluentinc/ksql#4260
Tested with: