Skip to content

Conversation

@cooper3330
Copy link
Contributor

Issue #, if available:

#913

Description of changes:

This updates the docker create command to configure the proxy settings inside the docker container using global docker configuration file. This will allow users who sit behind a proxy and have configured their proxy settings in docker to pass through their proxy for things such as dependency installation (Pip, etc.).

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sriram-mv
Copy link
Contributor

Thanks for the contribution! 💯

On first pass this looks good. I have some follow up questions

@cooper3330
Copy link
Contributor Author

Testing this is an interesting case. Since this is only modifying the Docker Container Create SDK call, I think we just need to test that the container.create SDK call is kicked off with the expected parameter. It is equivalent to including the 'tty=False' in the container create call.

To test these today, we are verifying the Container Create call was kicked off with the correct parameters passed. The proposed PR maintains that standard of testing with the underlying Docker SDKs by asserting docker.create passed in the use_config_proxy=True parameter.

Any issues/disagreement with that train of thought?

@sanathkr
Copy link
Contributor

sanathkr commented Jun 6, 2019

@cooper3330 Your testing rationale makes sense.

I digged thru Docker-Py's recent commit of docker/docker-py#2202 to understand the implementation. I am confident that this doesn't have any side effects until you set proxy config in global Docker configuration files.

But this feature is available in Docker SDK >= 3.7.0. We need to update the version in requirements/base.txt

sanathkr added a commit to sanathkr/aws-sam-cli that referenced this pull request Jun 6, 2019
@sanathkr
Copy link
Contributor

sanathkr commented Jun 6, 2019

Bumping docker version separately - #1214 so we know this is safe before merging this.

@sanathkr sanathkr merged commit ee06566 into aws:develop Jun 7, 2019
@cooper3330 cooper3330 deleted the docker-proxy-config branch June 10, 2019 15:58
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

Successfully merging this pull request may close these issues.

3 participants