Skip to content
This repository has been archived by the owner on Dec 4, 2020. It is now read-only.

Added support for docker4mac #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added support for docker4mac #148

wants to merge 1 commit into from

Conversation

mcroker
Copy link

@mcroker mcroker commented Sep 29, 2016

Added support for docker4mac (which doesn't use docker-machine)... invoked by quickstart -t mac.

@nickdgriffin
Copy link
Contributor

nickdgriffin commented Sep 30, 2016

Hi Martin,

The current compose init already supports not using docker-machine, that's what PROXY_IP was put in for. Running on a Mac is the same as running locally I expect, which is what Travis does (as it can't use Machine with VirtualBox). Also, the intention of quickstart.sh was meant to be just for the case of using docker-machine in front to provision a machine. Natively I'd expect just the CLI to be used.

Are there other considerations needed for running on docker4mac? Also, judging by the fact it hasn't merged cleanly I think perhaps you need to refresh your branch from master?

@anton-kasperovich
Copy link
Contributor

anton-kasperovich commented Sep 30, 2016

Hi Martin & Nick,

I assume we can improve this PR a bit more, to support both Mac & Windows at the same time, as starting from Windows 10 it's also possible to use init script without Docker Machine, Docker Native etc... I'm thinking that we can make QuickStart works in a two cases like with/without machine:

./quickstart.sh -t local -m adop_local
./quickstart.sh -t native
./quickstart.sh -t aws

What do you think?

@mcroker
Copy link
Author

mcroker commented Sep 30, 2016

Anton, I agree completely and actually think '-t local' should possible be changed to '-t dockermachine' (but left it for backward compatibility). '-t native makes sense'

Nick, Ok - so does that mean my change to cmd/compose can be ignored and instead implemented through a further change to use PROXY_IP ? (perhaps you do do this as you merge ;) )?

Martin

@nickdgriffin
Copy link
Contributor

PROXY_IP was put in for the case where the stack would be accessed from a custom IP/domain (such as behind an SSL-enabled ELB), and the fall through in case Docker Machine wasn't being used so it defaults to localhost was put in place for Travis (https://github.com/Accenture/adop-docker-compose/blob/master/.travis.yml#L22). I thought the order of precedence should be custom domain > Docker Machine > local Engine.

What was the intended outcome of your change that isn't currently covered by the logic? Is there a gap here when working locally that I'm missing?

@mcroker
Copy link
Author

mcroker commented Oct 3, 2016

If PROXY_IP is not set the cmd/compose script assumes use of docker-machine. It fails with error if a docker-machine is specified that doesn't exist (or no machine is set and the default machine doesn't exist). My change was to override this behavior where no machine is specified (i.e. remove the assumption of a default docker-machine.

The suggestion I made above was to leave cmd/compose unmodified - and have quickstart specify IP_PROXY as localhost where '-t native' is specified in the options.

@nickdgriffin
Copy link
Contributor

It wasn't expected that a machine name would be specified that was invalid so that's why the logic doesn't handle that in a useful way other than just bottling out - it assumes a mistake or issue, rather than falling through to localhost. If the -m flag isn't provided then it should fall through to defaulting to localhost as that's how it works in Travis.

I think having "-t native" explicitly setting "-i localhost" is a good way to feed this in and make it clear to anyone who looks into the quickstart what is expected, instead of having to dive into "cmd/compose" which is a bit more complex.

@mcroker
Copy link
Author

mcroker commented Oct 4, 2016

'-t localhost' perhaps... but otherwise lgtm...

@nickdgriffin
Copy link
Contributor

"-t localhost" is more logical.

@luismsousa
Copy link
Contributor

is this still an issue? can it be closed?

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

4 participants