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

Move checking Docker connectivity to ContainerManager #828

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@ndobryanskyy
Copy link
Contributor

ndobryanskyy commented Dec 4, 2018

Description of changes:

Eliminated _check_docker_connectivity() method and introduced new property in ContainerManager is_docker_reachable. Also now ContainerManager is created on __enter__ by InvokeContext.

Updated tests and added new as required. This is the first part of porting enhancing changes from #774 .

I think we could close #774 now, right?

Checklist:

  • Implement required changes
  • Write unit tests
  • make pr passes

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

@jfuss jfuss self-requested a review Dec 5, 2018

@jfuss jfuss self-assigned this Dec 5, 2018

@jfuss

jfuss approved these changes Dec 5, 2018

Copy link
Contributor

jfuss left a comment

Thanks for porting the improvements out into this PR!

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 5, 2018

@jfuss more to come 😉

self._skip_pull_image)

if not self._container_manager.is_docker_reachable:
raise InvokeContextException("Running AWS SAM projects locally requires Docker. Have you got it installed?")

This comment has been minimized.

@TheSriram

TheSriram Dec 5, 2018

Contributor

I wonder if we can say Docker > x version, I think that requires some investigation though.

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Dec 5, 2018

Contributor

@TheSriram SAM CLI is constrained to some Docker version?

This comment has been minimized.

@TheSriram

TheSriram Dec 5, 2018

Contributor

Not really, just a way to ask people to upgrade to the latest. There have been some instances where users were using a really old version of docker and had to upgrade. example: to get mounting to work as expected.

@jfuss jfuss merged commit 19297dd into awslabs:develop Dec 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

j0lly added a commit to j0lly/aws-sam-cli that referenced this pull request Dec 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment