projectatomic / vagrant-service-manager

To provide the user a CLI to configure the ADB/CDK for different use cases and to provide glue between ADB/CDK and the user's developer environment.
GNU General Public License v2.0
18 stars 16 forks source link

Align proxy config names similar to 'sccli' #434

Closed coolbrg closed 7 years ago

coolbrg commented 7 years ago

Currently, proxy configurations names are http_proxy, http_proxy_user and http_proxy_password which seems to specific to http and may confuse user to think about different configurations for https proxy.

Let's use config names as proxy, proxy_user and proxy_password as used by sccli here.

Also, proposing this change as I found bug in setting proxy through service-manager that proxy is not being setup properly.

Command being executed through service-manager is

HTTP_PROXY='url' HTTP_PROXY_USER='username' HTTP_PROXY_PASSWORD='password' sccli openshift

which is a wrong command. Actual command should be used is:

PROXY='url' PROXY_USER='username' PROXY_PASSWORD='password' sccli openshift
hferentschik commented 7 years ago

Currently, proxy configurations names are http_proxy, http_proxy_user and http_proxy_password which seems to specific to http and may confuse user to think about different configurations for https proxy.

These are the standard names on OS X / Linux (see for example curl). For me the http prefix makes sense and I don't think that there is a risk for of confusion with HTTPS.

That said, I don't have strong feelings one way or another, but we also need to realize that we already did a release with the properties with http prefix. One can not just change the properties now. If you want to do it properly you need to accept both for some time.

Command being executed through service-manager is HTTP_PROXY='url' HTTP_PROXY_USER='username' HTTP_PROXY_PASSWORD='password' sccli openshift which is a wrong command.

So we did not really test this, again? Another usecase for a test proxy!?

@LalatenduMohanty What's your take on this?

hferentschik commented 7 years ago

Merged via pull request #435