Skip to content
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

Check Docker version according to checksEnabled flag #1231

Merged

Conversation

aivinog1
Copy link
Contributor

@kiview
Copy link
Member

kiview commented Feb 11, 2019

Thanks, for the PR.
I think, the change is actually a bit big, considering what we want to achieve.

IMO it's enough to move


to line 132, inside the if body.

@testcontainers testcontainers deleted a comment Feb 11, 2019
@testcontainers testcontainers deleted a comment Feb 11, 2019
@testcontainers testcontainers deleted a comment Feb 11, 2019
@testcontainers testcontainers deleted a comment Feb 11, 2019
@aivinog1
Copy link
Contributor Author

@kiview Thank you for review. I think you are fair enough. But, may be we need some refactoring to move checks in another class. That will really help us if checks start to grow.

What do you think about it? If you agree I (or maybe you) can create task to do this refactoring.

@testcontainers testcontainers deleted a comment Feb 12, 2019
@kiview
Copy link
Member

kiview commented Feb 12, 2019

@aivinog1 Having a dedicated environment checking class seems to make sense. I would move all checks there in this case.

However, for now this fix is good to go 🙂

@kiview kiview added this to the next milestone Feb 12, 2019
@kiview kiview changed the title #1220 Added a new setting which disable docker version validation. #1220 Check Docker version according to checksEnabled flag Feb 12, 2019
@kiview kiview changed the title #1220 Check Docker version according to checksEnabled flag Check Docker version according to checksEnabled flag Feb 12, 2019
@kiview kiview merged commit 375bdae into testcontainers:master Feb 12, 2019
@kiview
Copy link
Member

kiview commented Feb 12, 2019

Merged, thanks a lot for your first contribution!

kiview added a commit that referenced this pull request Feb 12, 2019
@rnorth
Copy link
Member

rnorth commented Mar 12, 2019

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.

None yet

3 participants