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

Erratic behavior with method options due to Docker version differences #23

Closed
xeger opened this issue Jun 4, 2016 · 8 comments
Closed

Comments

@xeger
Copy link
Owner

xeger commented Jun 4, 2016

Docker is adding features steadily and the way our gem is structured, we always pass a value for every option. This causes Docker::Compose methods to fail if the Docker Engine version is older than expected.

We need a generalized fix. Some options:

  1. Don't add CLI options unless they vary from the default (users must opt into newish flags)
  2. Build a map of option-to-version and filter out unsupported options -- ugh!
  3. Enable Docker::Compose to download suitable binary on the fly -- seems too magical
  4. ???
@xeger
Copy link
Owner Author

xeger commented Jun 4, 2016

@CpuID your opinion?

@CpuID
Copy link
Contributor

CpuID commented Jun 4, 2016

@xeger I was thinking more about this overnight and realised yea a generalized fix would be wise - the issue with .Mounts here was the first example but I suspect not the last.

I am interested in how you would implement Option 1 in more detail, ie what input method a user would use to specify wanting a new flag. If it's relatively painless this might be a clean approach.

Option 2 feels like the most seamless from a users perspective, but the highest maintenance burden for the gem. I would think you would need to have a general semver dependency gem and explicitly enable/disable certain flags based on maj/min/rev etc (would involve keeping an eye on the Docker Compose changelogs). At the same time if there is a subsystem for handling this within the gem, contributors may PR things that go unnoticed at first, and the PRs should be relatively small each time.

Option 3 does feel quite magical, and I suspect may be pushing the limits of the responsibilities of the gem. Not to mention then you start requiring cross platform install helpers and stuff, can become quite a bit of maintenance work in its own right. The other thing would be, do you provide a user the ability to specify the Docker Compose version on instantiation, and use a sane default version (which would need bumping every now and then?). One big issue I can see here is the gem is currently close to thread/multiprocess safe as it defers logic to docker-compose. My current use case actually involves parallel rspec processes using parallel_tests, and the first process does the stop/rm/up and subsequent processes will just run a ps to grab the state of containers. In this scenario I would need to ensure that instantiation in the secondary processes don't all try and install docker-compose in parallel and pave over eachothers work (lockfile maybe?).

After writing the comments above and brainstorming a little more, Options 1 and 2 feel the most sane, all depends if we want to put the work on the users or the maintainers/contributors :)

@xeger
Copy link
Owner Author

xeger commented Jun 5, 2016

The idea with option 1 is that the gem would "know" about the options as of a specific d-c version, and therefore know about the default values for those options (as encoded in keyword arg default values). However, if a user didn't pass a kwarg, or passed a value identical to the default, then the gem would simply omit that flag when invoking d-c and allow it to assume the default value. Thus, users with an older d-c version wouldn't be impacted until they tried to use an option that is unsupported in their version.

Option 2 is attractive to some degree, but we'd need to figure out how the gem should behave when a user applies an option that's unsupported in his version; we'd need to update the gem frequently as d-c evolves.

Both options have the shortcoming that d-c might get newer than the gem, in which case there'll be options that we have not had a chance to encode into the gem, and therefore no way to use those options.

Both options would probably benefit from version synchronization between the gem and the tool, i.e. v1.7.x of the gem would know about the options set of v1.7 of the tool. The session constructor could do a version check and emit a warning if the tool's major/minor version is different from the gem's. Not quite full semver awareness, but it involves a good bit less work for we maintainers than option 2.

If we need the gem to be future-proof, there's always option 4: transforming the params of every method into a *args glob with as many positional and keyword params as the user wants to cram in. The downside of this approach is that it becomes more difficult for us to document the options and their defaults, and the Ruby interpreter will no longer stop you from making a method call (and thus a tool invocation) with nonsensical options. I kind of like the property that the method decls do some error checking right now.

I s'pose Option 1, plus a version check at startup and some semver alignment between the gem and tool, is a reasonable path for now. We could always devolve to Option 4 at some later date.

Re versioning, the gem is at 0.x because it lacked tests and docs, and I wasn't sure whether it'd develop a user base. Provided that I keep test coverage up to snuff, it's probably time to step it to 1.x (which would handily align it with the tool version).

@CpuID
Copy link
Contributor

CpuID commented Jun 5, 2016

Sounds good, Option 1 is feeling sane right now and yea its always possible to iterate towards Option 4 if it became necessary (I agree the sanity checks lost in Option 4 could potentially get annoying - being able to sanity check in the gem is nice in some ways, at the cost of gem maintenance).

@xeger xeger closed this as completed in d48ab53 Jul 29, 2016
@xeger xeger reopened this Jul 29, 2016
@xeger
Copy link
Owner Author

xeger commented Jul 29, 2016

@CpuID added provisional support for this in 1.0.0rc1

@EugenMayer
Copy link
Collaborator

maybe i miss the point, why do not just pass on the args rather then try to filter anything? Let the user/implementor decide, what he wants to pass and what ENV he expects, instead of trying to create and overlay? Maybe i just missed the point here though.

@xeger
Copy link
Owner Author

xeger commented Jul 29, 2016

The problem is that Docker keeps adding options and then removing them. Let's take --no-build as an example. They added it in 1.3 and removed it in 1.6 (it became the default, and they added --build instead.)

Our gem added the no_build: option at some point in order to be compatible. However, because our gem always passes a value for any option it knows about, that means our gem always passed a --no-build false. When Docker removed that option, our gem became broken vs. the newer Compose.

The converse can also happen: our gem might know about a new option such as stop --timeout, but it always passes the --timeout option; if you use it with an older Compose version, then Compose is broken vs. our newer gem.

Because Docker moves so quickly and does not follow semver (they add and remove options in the 1.x series of their software), our gem needs to adapt.

Step 1 of adapting is to allow a newer gem with an older Compose provided you do not want to make use of new options. Our gem knows that the default value of --no-build is false; unless you pass true to the Ruby method, there is no need to include a --no-build flag on the command line (default behavior is OK). Of course, if you need to pass no_build: true, then we cannot help you -- but at least we can help you if you don't care about no-build.

Step 2 of adapting is to allow an older gem to work with options of a newer Compose. Unfortunately, the only way to do that is to use a keyword args glob:

def up(*services, opt1: false, opt2: 10, **unknown)
  # translate unknown keywords into flags using sugar
end

The problem with this, is that we lose the "self documenting" beauty of the gem -- the user can't look at the rubydoc to discover flags, default values, etc -- and if the user passes a bogus flag, or a flag that is only available with newer Compose, he will receive a confusing error from Compose.

Anyhow, I know how to solve half the problem, so I've included the step-1 fix in the 1.0 rc1 release to see how it works.

@xeger
Copy link
Owner Author

xeger commented Sep 3, 2016

Fixed in 1.0.0

@xeger xeger closed this as completed Sep 3, 2016
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

No branches or pull requests

3 participants