ContainerSolutions / minimesos

The experimentation and testing tool for Apache Mesos - NO LONGER MAINTANED!
https://www.minimesos.org
Apache License 2.0
427 stars 61 forks source link

#227 - don't store environment map #402

Closed adam-sandor closed 8 years ago

adam-sandor commented 8 years ago

Looking at the description of #227 I think the main problem is not a lack of setters (which can be added of course if needed) but the fact that the environment map is stored as a field in AbstractContainer. As it is only a map of constants and derived values storing it doesn't add value but can lead to problems if it becomes out of sync with the primary values. There is also a quite complex relation between AbstractContainer and its subclasses making it hard to track how the final map gets assembled.

What I tried to do is:

  1. Remove the AbstractContainer.envVars field + the conversion method that produces the String[]
  2. Create the EnvironmentBuilder class to add a nice syntax for assembling an environment map from multiple sources.
  3. Assemble the final map just before passing it to the createContainerCmd. This way I managed to avoid having to override methods to add values to the environment map. I think this makes it easier to follow what's going on in the code. There is one method for creating the shared variables and both the master and the agent add their own to those. I think the biggest improvement is the ConsulContainer where a new variable is added to the envVars map just to be used a few lines later by a method that delegates to the AbstractContainer, this was the inspiration for the change to create the map right where it get's used.
sadovnikov commented 8 years ago

@adam-sandor, Travis CI build on your PR fails because out authentication token, which is set via environment variables, is not available to unknown code. See https://docs.travis-ci.com/user/environment-variables/#Encrypted-Variables

Encrypted variables are not added to untrusted builds such as pull requests coming from another repository... Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code

sadovnikov commented 8 years ago

@adam-sandor, could you update travis.sh in your fork by copying content from https://github.com/ContainerSolutions/minimesos/blob/build/removal-of-jenkins/travis.sh ? This should fix builds of your PR

remmelt commented 8 years ago

I like this solution better than adding setters, because this is more flexible. Nice!

sadovnikov commented 8 years ago

@adam-sandor, thanks for adding the test and answering my question. Unfortunately now your PR conflicts with changes in our repository (most probably due to merge of removal-of-jenkins branch I asked you to copy travis.sh from). Would you mind resolving the conflict?

If it's too much, I'll resolve it myself, but then it stops being your PR and I'd prefer to keep your authorship ;-)

adam-sandor commented 8 years ago

I did the merge, let me know if there is anything more I should do.

sadovnikov commented 8 years ago

Approve

sadovnikov commented 8 years ago

Approved

Approved with PullApprove