Generate configuration file with envsubst #25
Conversation
We added a `set -u` in previous commit which causes a "unbound variable" failure if some environment variables weren't defined.
This will allow us to run the image easily with just mysqld and other parameters, which is generally useful for debugging/testing/experimenting.
5.5/run-mysqld.sh
Outdated
There was a problem hiding this comment.
rather than doing this, why not just default the values in the template file?
There was a problem hiding this comment.
nm, i see envsubst doesn't support defaults.
mnagy
force-pushed
the
config_customization
branch
from
b23fb44 to
778d5b2
Compare
|
Added rest of options that we had in v2 and some tests. PTAL |
|
looking good @mnagy, i especially like the tests and doc! |
|
Thanks! Docs where shamelessly stolen from v2 MySQL cartridge docs :) |
|
LGTM |
5.5/contrib/my.cnf.template
Outdated
There was a problem hiding this comment.
I still don't like this. We should use the env vars wisely to provide values to configuration directives... not to generate the directives themself. Can't you just add default-time-zone =${TZ} ? What happen when this setting will be just set to blank value? Is that default?
|
NACK, I don't want us to use environment variables to contain the complete configuration directives as that is not clear and might be confusing. We need to stick to use them only as values for those directives. Even if that means that we will have to find some reasonable defaults for those ourself. Don't overuse envsubst to do templating. |
|
I've must have missed that one taking back my LGTM, sorry. I agree with On Wed, Mar 25, 2015 at 10:43 AM Michal Fojtik [email protected]
|
|
@soltysh @mnagy stick to customizations we can do easely with envsubts, everything else that require conditions or complex logic is out of scope. Users can provide their own my.cfg if they want just by downloading and customizing the image. So the simple rule here is if you want to do something that requires more logic than just setting value for config directive, it is out-of-scope. @bparees agree? |
mnagy
force-pushed
the
config_customization
branch
from
778d5b2 to
56da065
Compare
|
Timezone dropped as agreed with @mfojtik. PTAL |
mnagy
force-pushed
the
config_customization
branch
from
56da065 to
a9f107c
Compare
|
Added comments in the template with default values as requested offline by @mfojtik. |
README.md
Outdated
There was a problem hiding this comment.
this might break the VOLUME data if you update it for Pod. If the VOLUME already exists and it use ISAM and you change it to InnoDB the mysql won't start. Also in latest MySQL they are going to drop MyIsam so I think we should remove this option (dangerous and potentially going away in near future)
We will now generate the configuration file for every run of mysqld, using environment variables as input. Substitution is very simple, done with envsubst.
|
We could still leave the timezone config line in the file, commented out..at least that would serve as a hint to users who wanted to configure it. @mfojtik i'm not sure i agree..i'm ok with it of for now i guess but i'm not super happy about it, it's a regression from what we supported in v2. |
|
@bparees mysql allows to set timezone per connection or globally via SET TIMEZONE |
mnagy
force-pushed
the
config_customization
branch
from
a9f107c to
3085523
Compare
|
@mfojtik i'm not saying there aren't other ways to do it, but we did used to support it via config file too. |
|
Removed storage engine setting. |
|
@bparees I agree, but I think we should also not blindly copy things from v2 just because we used to have them there. For example, users can set 'TZ' variable and mysql will pick it up during start. I'm just saying that initially, we don't need to have full-feature set for things we did support in v2, because the v2 was limited in what you can do in terms of customization. TZ might be one example... in V2 I don't think we allowed to override it because it will also be set for the application and other things that runs in the gear. |
|
/me is convinced by @mfojtik |
|
LGTM |
1 similar comment
|
LGTM |
Generate configuration file with envsubst
Add findutils to list of packages
5 participants
This is my first take on https://trello.com/c/NGcr5u20/534-5-database-image-configuration-details-image-beta3
I added two settings from v2 MySQL cartridge. One,
MYSQL_MAX_CONNECTIONShas a default set and is simply substituted in the config file. The other,MYSQL_TIME_ZONEis only used if it set, otherwise a commented entry is added into the config file for clarity.Comments and suggestions are wanted, after this gets merged I'll do the same for PostgreSQL.