Add EnvironmentLoader to customize how Migrations loads environment settings. #128

harawata · 2018-09-04T20:11:32Z

There has been a few requests for alternative methods to configure environment settings (#90 , #122).

This PR adds a new interface EnvironmentLoader.
Migrations looks for its implementation via Java SPI at runtime (similarly to #107).

public interface EnvironmentLoader {
  Environment load(String environmentName, SelectedPaths paths);
}

@h3adache ,
I do not fully understand how you use Migrations, so please take a look and see if it meets the requirements (no rush, of course).
Here is the implementation that I used to test.

public class JarEnvironmentLoader extends PropertiesEnvironmentLoader {
  private static final Path ZIP_PATH = Paths.get("/.../environments.zip");

  @Override
  public Environment load(String environmentName, SelectedPaths paths) {
    String filename = "/environments/" + environmentName + ".properties";
    try (FileSystem fs = FileSystems.newFileSystem(ZIP_PATH, ClassLoader.getSystemClassLoader())) {
      Path path = fs.getPath(filename);
      try (InputStream is = Files.newInputStream(path)) {
        Properties properties = new Properties();
        properties.load(is);
        return buildEnvironment(properties);
      }
    } catch (IOException e) {
      throw new MigrationException("Environment file missing: " + filename + " in " + ZIP_PATH, e);
    }
  }
}

I'll add documentation if you approve the change.

…ettings.

h3adache · 2018-09-04T23:29:20Z

Hi @harawata! Sorry I've been busy and meant to do the work for this myself. I was thinking something more simple on the lines of Resources or as described on ResolverUtil.
We use a similar concept on mybatis spring already (but using springs resource loaders). This allows people to load environments files that are bundled into their jar or somewhere else in their classpath.

This allows me to create and distribute a jar that can both up and down migrations for multiple environments.

I see the benefit of your approach is that you can load from any resources even a central config server so that's pretty neat in itself.

harawata · 2018-09-05T14:33:24Z

I see.
So, were you planning to introduce some special prefix like classpath: to specify resource path (e.g. --env=classpath:/environments/development.properties) ?

I see the benefit of your approach is that you can load from any resources even a central config server so that's pretty neat in itself.

It is flexible for sure, but it also feels like a little bit too over-engineered.

h3adache · 2019-07-03T12:46:52Z

@harawata sorry for the late review.
Mostly just code consistency/style changes but the main comment is about being able to compose these environmentloaders.
It works well with what I had (I can refactor that to use this style) if we did it that way I think.

Order of checking would go Properties -> System (with prefix) -> Environment (with prefix) -> user custom that would seem to solve all our needs?

h3adache · 2019-07-03T12:47:39Z

FYI I made the style/code consistency changes already if you want me to push those up to a shared branch? Just to save some time.

harawata · 2019-07-03T13:08:04Z

It works well with what I had (I can refactor that to use this style) if we did it that way I think.

I would be happy to take a look at your approach if it's simpler.
I am still not sure if this level of flexibility is necessary, to be honest.

if you want me to push those up to a shared branch? Just to save some time.

Sure, you can push directly to my branch, right?

harawata · 2019-07-03T13:09:41Z

Order of checking would go Properties -> System (with prefix) -> Environment (with prefix) -> user custom that would seem to solve all our needs?

I think so.

h3adache

It works well with what I had (I can refactor that to use this style) if we did it that way I think.

I would be happy to take a look at your approach if it's simpler.
I am still not sure if this level of flexibility is necessary, to be honest.

I'll add the changes on top of yours.

if you want me to push those up to a shared branch? Just to save some time.

Sure, you can push directly to my branch, right?

I cant seem to push to your branch so I pushed to a same named branch on origin

h3adache · 2019-07-03T12:26:49Z

src/main/java/org/apache/ibatis/migration/commands/BaseCommand.java

-    environment = new Environment(existingEnvironmentFile());
+    EnvironmentLoader envLoader = null;
+    for (EnvironmentLoader found : ServiceLoader.load(EnvironmentLoader.class)) {
+      if (envLoader != null) {


We should allow multiple loaders using some default fallbacks

Any example use case of multiple loaders?

Sorry I meant this to be from 213-222.
So removing the null check on 220 and adding their custom loader on top of set default ones (similar to spring properties resolver).
I am changing lines 220-223 to add my environment and system properties loaders.

h3adache · 2019-07-03T12:33:47Z

src/main/java/org/apache/ibatis/migration/commands/InitializeCommand.java

@@ -43,7 +43,8 @@ public void execute(String... params) {
    createDirectoryIfNecessary(paths.getDriverPath());

    copyResourceTo("org/apache/ibatis/migration/template_README", Util.file(basePath, "README"));
-    copyResourceTo("org/apache/ibatis/migration/template_environment.properties", environmentFile());
+    copyResourceTo("org/apache/ibatis/migration/template_environment.properties",
+        new File(paths.getEnvPath(), options.getEnvironment() + ".properties"));


just a code consistency here. but envpath can be extracted.
also the rest use util.file (which can really just be changed to what you have here File(parent, file) or removed)

h3adache · 2019-07-03T12:35:05Z

src/main/java/org/apache/ibatis/migration/commands/InitializeCommand.java

@@ -43,7 +43,8 @@ public void execute(String... params) {
    createDirectoryIfNecessary(paths.getDriverPath());

    copyResourceTo("org/apache/ibatis/migration/template_README", Util.file(basePath, "README"));
-    copyResourceTo("org/apache/ibatis/migration/template_environment.properties", environmentFile());


this method isn't used anymore. nor the supporting existingEnvironmentFile

h3adache · 2019-07-03T12:38:13Z

src/main/java/org/apache/ibatis/migration/PropertiesEnvironmentLoader.java

+    hook_after_new
+  }
+
+  protected static final List<String> SETTING_KEYS;


I thought that users might create a custom loader by subclassing this built-in one.

Ah that makes sense but I think if that's the case we should move this to something common.
This is the keys though, it is an implementation detail that we have changed before so depending on it might not be a good idea

h3adache · 2019-07-03T12:38:30Z

src/main/java/org/apache/ibatis/migration/PropertiesEnvironmentLoader.java

+  protected Properties readProperties(String environmentName, SelectedPaths paths) {
+    File file = new File(paths.getEnvPath(), environmentName + ".properties");
+    FileInputStream inputStream = null;
+    try {


can be replaced with try with resources

h3adache · 2019-07-03T12:40:03Z

src/main/java/org/apache/ibatis/migration/PropertiesEnvironmentLoader.java

+      Properties prop = new Properties();
+      prop.load(inputStream);
+      return prop;
+    } catch (FileNotFoundException e) {


Should this just be logged warning?

If a user specified --env=someenv and there is no someenv.properties, throwing an exception seems appropriate.
What kind of use case are you thinking about?

So, you want Migrations to work even if there is no .properties file, am I right?

yup! I'm ok either way since there's a default one that's added during init.
So it's not a big deal. more for flexibility

I think it's a good idea.
We can, for example, ignore FileNotFoundException only when the environmentName is "development".

h3adache · 2019-08-20T12:36:23Z

Can this be closed now @harawata?

harawata · 2019-08-20T12:52:53Z

@h3adache ,
Yup! And thank you for sharing your wisdom on this topic!

Add EnvironmentLoader to customize how Migrations loads environment s…

ef107db

…ettings.

harawata requested a review from h3adache September 4, 2018 20:11

h3adache reviewed Jul 3, 2019

View reviewed changes

harawata mentioned this pull request Jul 13, 2019

Allow more flexible environment settings #146

Merged

harawata closed this Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnvironmentLoader to customize how Migrations loads environment settings. #128

Add EnvironmentLoader to customize how Migrations loads environment settings. #128

harawata commented Sep 4, 2018

h3adache commented Sep 4, 2018

harawata commented Sep 5, 2018

h3adache commented Jul 3, 2019

h3adache commented Jul 3, 2019

harawata commented Jul 3, 2019

harawata commented Jul 3, 2019

h3adache left a comment •

edited

Loading

h3adache Jul 3, 2019

harawata Jul 3, 2019

h3adache Jul 8, 2019

h3adache Jul 3, 2019

h3adache Jul 3, 2019

h3adache Jul 3, 2019

harawata Jul 5, 2019

h3adache Jul 8, 2019

h3adache Jul 3, 2019

h3adache Jul 3, 2019

harawata Jul 5, 2019 •

edited

Loading

harawata Jul 10, 2019

h3adache Jul 12, 2019

harawata Jul 12, 2019

h3adache commented Aug 20, 2019

harawata commented Aug 20, 2019

Add EnvironmentLoader to customize how Migrations loads environment settings. #128

Add EnvironmentLoader to customize how Migrations loads environment settings. #128

Conversation

harawata commented Sep 4, 2018

h3adache commented Sep 4, 2018

harawata commented Sep 5, 2018

h3adache commented Jul 3, 2019

h3adache commented Jul 3, 2019

harawata commented Jul 3, 2019

harawata commented Jul 3, 2019

h3adache left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harawata Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3adache commented Aug 20, 2019

harawata commented Aug 20, 2019

h3adache left a comment •

edited

Loading

harawata Jul 5, 2019 •

edited

Loading