optionally add docker host to proxy excludes in env command

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍

[databus23]

This adds a --no-proxy flag to the docker-machine env command.
When set it will take care of adding (and removing) the docker host to/from the NO_PROXY environment variable.

This helps using docker-machine with docker >=1.5 with local vm providers (e.g. fusion, virtualbox) in coporate networks where an http proxy is required for internet connectivity.
Since version 1.5 the docker client picks up the HTTP_PROXY environment variable when connecting to the docker daemon. This does not work well for the local vm use case. In that case the docker host is inside a local virtual network only reachable from the workstation itself.

@ehazlett This pull request also has the intention to illicit some feedback from you regarding this PR boot2docker/boot2docker-cli#345 :)

[ehazlett]

@SvenDowideit your thoughts?

[nathanleclaire]

I haven't had time to review this but @databus23 I'd suggest you please break this down into several smaller functions- that's a lot of stuff for just one function to do.

[databus23]

@nathanleclaire Sure. I agree its quite a lot of code for one method and can be structured better. But before I spend more time on this can we agree that there is a problem and a solution would be considered for a merge?

[ehazlett]

@databus23 yes i think this would be beneficial

[nathanleclaire]

@databus23 Yes, I am in favor of the feature.

I have a question: If the user wants to connect to a remote instance, they will actually want to use the proxy to enable internet connectivity, no? How is that case handled?

Also, side note for @ehazlett : this logic is really getting hairy enough that we should eventually move to using a text/template to generate these commands instead.

[ehazlett]

+1 for text/template -- there are a couple other places that could use that too.

[databus23]

@nathanleclaire Yes, for remote instances the user might actually need the proxy to get to the instance. Thats why I implemented it as an optional boolean flag --no-proxy. So the user has to explicitly set it e.g. docker-machine env dev --no-proxy. An optimisation could be to automatically set it for local vm providers (fusion, virtualbox, hyperv?) where using the proxy most certainly won't work.

[nathanleclaire]

@databus23 Ah, of course - the flag. I'm sorry, I'm being thick due to this recent flurry of activity. At any rate, if you break that into a few smaller functions and especially if you add some unit tests (for these new functions - don't feel responsible for cmdEnv in general), this PR will be looking in really good shape from my end.

[nathanleclaire]

Some guidance on how to structure the subroutines: please pass in the values from c.Bool() calls directly (to avoid passing around the whole context object), as well as machineUrl and the variable for the shell.

[databus23]

@nathanleclaire I'm not sure I understand your guidance, could you maybe elaborate?

[nathanleclaire]

I'm not sure I understand your guidance, could you maybe elaborate?

Most important thing is to parse what you need out of cli.Context struct instead of passing it around. We are trying to stop passing c to functions since IMO it shouldn't be needed outside of the cmd* functions.

[nathanleclaire]

@databus23 Any followup on this?

[databus23]

Not yet. But I'm still interested in seeing this in machine. #1676 is kind of a duplicate of this issue btw.

[databus23]

I updates the pull request against latest master. This will add the docker host ip to the no_proxy environment variable if requested via docker env $NAME --no-proxy.
The unset case is not handled, so the ip will remain in the no_proxy environment variable in case docker env $NAME --unset is used. It's not that easy to add given the current way the text template is rendered. I don't think this is a show stopper though. In almost all cases the exclucded ips will be from private address spaces used by local vm providers on virtual network interfaces. Having an old entry lurking around in the no_proxy environment variable shouldn't be a big problem.
IMHO its definitely a smaller issue than docker machine not working with local vm providers in environments where an http_proxy is mandatory for internet access.

This fixes #1676 which has ben labeled help-wanted by @ehazlett.

Thoughts?

[nathanleclaire]

Nice, I'll make sure to take a look.

[databus23]

Rebased on top of current master. Circles is still in a moody.

[databus23]

Rebased on current master. All green.

[nathanleclaire]

This is rad! Code looks pretty good. Could I get you to add a section to docs/env.md explaining this feature? Otherwise I worry people will not be able to find it.

[databus23]

@nathanleclaire documentation added.

[nathanleclaire]

Alright, sorry about this but I've just merged the rather large libmachine PR as well, which will introduce some conflicts. I'll start working on rebasing this branch (want to get it merged quite soon) to that and then making a PR on your repo, feel free to do it yourself as well if you have time. You can reach me on IRC to ask questions about the changes if you like: nathanleclaire

[nathanleclaire]

Update: I have successfully rebased so you won't need to worry about that. I will make a PR on your repo which you can merge (as well as add a few integration tests for this feature), and then we can merge this upstream!

[databus23]

@nathanleclaire you beat me to it by minutes. :)

[nathanleclaire]

:D

[nathanleclaire]

Carried it in my own PR to make things a little easier 👍