Skip to content
This repository has been archived by the owner on Apr 29, 2024. It is now read-only.

Support docker-compose variable substitution #17

Merged
merged 4 commits into from
Jun 14, 2016
Merged

Conversation

seglo
Copy link
Contributor

@seglo seglo commented Jun 12, 2016

This PR will allow the user to programmatically define a Map of key value pairs that are passed as environment variables to the call to docker-compose. A new setting has been added variablesForSubstitution that accepts a Map[String,String] to represent the variables.

https://docs.docker.com/compose/compose-file/#variable-substitution

@kurtkopchik
Copy link
Contributor

@seglo Thanks for the PR!

When implementing this feature I think we should take advantage of the fact that the plugin pre-processes the compose file and uses a modified version of the original compose file when executing the docker-compose commands.

So instead of having to modify a bunch of the code to pass the substitution variables around and how the processes are launched I think it would be easier to just replace the set of ''variablesForSubstitution' on the fly via RegEx in the readComposeFile code as follows:

  def readComposeFile(composePath: String, variables: Vector[(String, String)] = Vector.empty): yamlData = {
    val yamlString = fromFile(composePath).getLines().mkString("\n")

    val yamlUpdated = variables.foldLeft(yamlString) {
      case (z, (s, r)) => z.replaceAll("\\$\\{" + s + "\\}", r)
    }

    new Yaml().load(yamlUpdated).asInstanceOf[java.util.Map[String, java.util.LinkedHashMap[String, Any]]].asScala.toMap
  }

This would also allow the internal plugin that we use at Tapad (which derives from this plugin) to also use this feature when starting instances in our Mesos environment which doesn't use same "Process" starting code. The regex above could be made to be a little more robust but I think it handles the majority of cases. Let me know what you think. I can also make the modifications to your PR if you don't have the time. Just let me know.

Thanks again!

@seglo
Copy link
Contributor Author

seglo commented Jun 13, 2016

Hey @kurtkopchik . I was actually thinking the opposite: replacing the sbt-docker-compose substitution logic with this method to make the docker-compose.yml file reusable outside of the plugin (and vice-versa, if I use the variable substitution feature outside of the plugin and want to use it inside it too). As it stands now the file is incompatible unless you use sed or something to recreate the plugin's substitution logic. I was going to make the case after I submitted the PR to see how you might respond to this initial feature ;)

I wasn't aware that you had an internal version of this plugin that makes further use of the templating feature with your Mesos environment, or that it would break something more specifically. Would you be able to elaborate on that use case?

Either way, I'm fine if you decide to use the internal logic. I can probably take a look at it later this week if you like.

EDIT: "vice-versa" use case

@kurtkopchik
Copy link
Contributor

Hey @seglo. If you are just strictly using the environment variable substitution feature with the RegEx replacement implementation the same docker-compose file should work both with the plugin and without it. But if you are using some of the other plugin-specific substitutions tags such as <skipPull> or <localBuild> then, yes, you would have to replace those tags before using the file outside of the plugin.

Being able to do things like tag a specific set of images as <localBuild> within the compose file has been very useful for us when developing in sbt with a multi-project build. We also do some custom tagging to dynamically retrieve the latest version tagged images from our internal Docker registry on our internal version of the plugin. So I don't see the pre-processing "feature" of the plugin going away.

For launching compose instances in Mesos we leverage docker-compose-executor which we would also likely need to modify in some way to to inject the environment variables into the Process instance. If they are substituted in the compose file itself then there is no modification needed.

I hope that helps give a better explanation :) If you could take a look at using the internal logic version that would be awesome and much appreciated!

@seglo
Copy link
Contributor Author

seglo commented Jun 13, 2016

OK cool, I get it.

What if I supported both scenarios: substitution with traditional docker-compose variable substitution and your internal logic. A feature switch could be added with a default to your internal logic?

@kurtkopchik
Copy link
Contributor

I'd prefer to just have it use the internal logic version only for now to keep things simple.

Also wanted to note, for future PR's, that any changes to the RunningInstanceInfo class, which is serialized to disk to keep track of instances between sbt sessions would, require a change to the file name being used so that there would not be serialization conflicts between different versions of the plugin running on the same machine.

@seglo
Copy link
Contributor Author

seglo commented Jun 14, 2016

Cool. I updated the PR as you suggested. I reverted the changes to DockerCommands.

In regards to the RunningInstanceInfo class, what sort of strategy would you prefer when this datastructure is updated? Or would you prefer it's never updated? If I understand correctly, you're saying that when this datastructure is changed and a project uses a new version of the plugin then that serialized state will no longer be able to be deserialized.

* @param instanceData An optional parameter to specify additional information about the instance
*/
case class RunningInstanceInfo(instanceName: String, composeServiceName: String, composeFilePath: String,
servicesInfo: Iterable[ServiceInfo], instanceData: Option[Any] = None)
servicesInfo: Iterable[ServiceInfo], variables: Vector[(String, String)] = Vector.empty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should now be able to remove the previously added variables property and comment from RunningInstanceInfo.

@kurtkopchik
Copy link
Contributor

Thanks @seglo! I made a few comments and I'll merge the changes after the next set of updates.

Your understanding of RunningInstanceInfo is correct. I'm not completely opposed to updating it but wanted to do it infrequently as it would break compatibility. There are two options when updating.

  1. Update the filename that the data is serialized to. For example, append a major version number to it.
  2. Leverage the extra instanceData: Option[Any] field that exists on the class. The original intention was to store a data structure in this field that could be used to contain additional information about the instance without having to change the signature for RunningInstanceInfo but I haven't gotten around to actually defining exactly how to use it.

@seglo
Copy link
Contributor Author

seglo commented Jun 14, 2016

I think we're in business now. What do you think?

@kurtkopchik
Copy link
Contributor

Looks good! Thank you for the contribution. I'll be getting another official release out soon with this change included.

@kurtkopchik kurtkopchik merged commit 48b3f5c into Tapad:master Jun 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants