Skip to content
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

System/Environment variables #144

Closed
wants to merge 1 commit into from
Closed

System/Environment variables #144

wants to merge 1 commit into from

Conversation

h3adache
Copy link
Member

Don't merge this yet. Need tests

Add ability to define/override environment using system or environment variables.
Fixes 114

@h3adache h3adache requested a review from harawata March 24, 2019 03:07
@h3adache
Copy link
Member Author

WDYT @harawata?

This is semi more relevant now with the Docker migrations.
Also if used in CI/CD it's sometimes better to get sensitive items like passwords from the CI/CD system or some other secrets system.

@harawata
Copy link
Member

Hi @h3adache ,

Thanks for your work!
I agree that #114 (supporting environment variables/system properties) is a good enhancement. :)

Is your goal to omit .properties file completely?
If it is, I am OK, but it would be better to search variables with prefix including environment name (e.g.mybatis.migrations.development.username instead of username) to avoid unintentional overriding.

I was trying to implement it with a different approach.
It basically replaces variables in .properties file.
https://github.com/mybatis/migrations/compare/master...harawata:gh%2F114?diff=split&expand=1
Please ignore the test. I got busy and couldn't complete it.

@h3adache
Copy link
Member Author

No the goal was to do something more like Apache configurations or TypeSafe config which allows for system and environment level overrides. I can see what you mean by conflicts although in my years of using those other config systems I have not yet encountered any when there are sensibly named variables. We can namespace it using MyBatis? I think that’s more simple and convenient then the longer namespace. How you have ${} would be really nice for use in SQL but if the config system already contains them then we don’t need that

@harawata
Copy link
Member

With your approach, namespace/prefix is practically mandatory because there is no good workaround once the conflict occurs.
Also, we need to support multiple-environments case (i.e. development.properties, production.properties, ci.properties, ...) so that users can declare different environment variables for each environment.
So, the variable name will have to be like mybatis.production.password which is quite verbose.

With my approach, OTOH, choosing variable name becomes a developer's responsibility. =D
So, you can use shorter names if you know there is no conflict.

the goal was to do something more like Apache configurations or TypeSafe config which allows for system and environment level overrides.

Does this mean that you want to write some 'default' value in the .properties file and override it using environment variable?
Then it would be possible to enhance the variable replacement logic as we did in the core : mybatis/mybatis-3#852

@h3adache
Copy link
Member Author

With your approach, namespace/prefix is practically mandatory because there is no good workaround once the conflict occurs.

This is true. The config systems get around this by letting users decide what level or overrides they want to use as a configuration. We could do that as well.

Also, we need to support multiple-environments case (i.e. development.properties, production.properties, ci.properties, ...) so that users can declare different environment variables for each environment.
So, the variable name will have to be like mybatis.production.password which is quite verbose.
No we do not. This is over complicating and users decide what values to set in environment and system level so that's up to them not up to us. Keep the same keys for simplicity and ease of understanding.

With my approach, OTOH, choosing variable name becomes a developer's responsibility. =D
So, you can use shorter names if you know there is no conflict.

the goal was to do something more like Apache configurations or TypeSafe config which allows for system and environment level overrides.

I think this is directly related to what I said above about letting the overrides be configurable.
But I'm not saying that we shouldn't also support your ${xx} syntax. I think that's a very good system that's also familiar to people using frameworks like spring and even mybatis core. But that it can be in addition to this system of overrides. The concepts are complimentary not conflicting.

Does this mean that you want to write some 'default' value in the .properties file and override it using environment variable?
Then it would be possible to enhance the variable replacement logic as we did in the core : mybatis/mybatis-3#852

I think this is a good addition to your ${xxx} system. I do not agree that it replaces configuration values. Look at Spring application.properties and the config systems that I mentioned above. Some values feel more natural in configurations

@harawata
Copy link
Member

Okay. It might feel more natural to other users if it does to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants