fix(mysql-setup-job): add mysql default port override support #5036
Conversation
docker/mysql-setup/init.sh
Outdated
| (($MYSQL_PORT)) || port="3306" |
There was a problem hiding this comment.
I am not sure if this line is required or not. In the helm charts, the port param is defined and passed to this task.
See definition
See the env var passed to this job
Still, I couldn't be sure about the other deployment flows of datahub other then using helm. Also considering that the port param was not mentioned in the docs too, I thought it might be better to add a default value to here would be better instead of having an if statement and have two versions of the command mysql ... with -P and without it. Or should we just omit the possibility of this variable being undefined? WDYT?
There was a problem hiding this comment.
Can you try switching to bash by changing line 1 to #!/bin/bash
and then use this syntax to set defaults? https://github.com/datahub-project/datahub/blob/master/docker/kafka-setup/kafka-setup.sh#L2
There was a problem hiding this comment.
Nice suggestion, did it your way @dexter-mh-lee !
Could you take a look once again please?
Mysql setup job is given with a port override to use from helm charts but the job itself does not read that variable. Due to this, it is not possible to setup a MySQL DB if your cluster is serving from a port other than 3306(which is the default port of MySQL)
In this pr, I'm adding the port parameter, which should be already present when using helm charts, to the mysql-setup-job's connection parameters.
There is also an issue with the documentation of the port parameter for the MySQL. This is also handled in this pr
Checklist