Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Allow to use latest Docker (v 17.xx and later) in build #997

Merged
merged 1 commit into from
Mar 7, 2017

Conversation

msterin
Copy link
Contributor

@msterin msterin commented Mar 6, 2017

The version checking scheme in build.sh was not handling new Docker major versions (i.e. 17....)
well so the build was failing with "Docker 1.8 or later is required".
Fixed the version checking scheme to work properly.
We probably do not need this at all since Docker version < 1.8 is fairly ancient, but keeping in the code out of pure paranoia.

The version checking scheme in build.sh was not hanling new Docker major versions
well so the build was failing with "Docker 1.8 or later is required".
Fixed the version checking scheme to work properly.
We probably do not need this at all since Docker 1.7 is failry ancient, but keeping in the code
out of pure paranoia.
@msterin msterin added this to the 0.13 milestone Mar 6, 2017
@msterin msterin added the P0 label Mar 6, 2017
@msterin msterin self-assigned this Mar 6, 2017
Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for supplying a fix quickly.

if [ $min_ver -lt 8 ]
maj_ver=`echo $ver | sed 's/\..*$//'`
min_ver=`echo $ver | sed 's/[0-9]*\.//' | sed 's/\..*$//'`
if [ $maj_ver -lt 17 -a $min_ver -lt 8 ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm Is this condition correct? maj_ver < 17 && min_ver < 8? Why?

I think we can get rid of this code altogether and just document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this condition means 1.7 and below :-). See description - yes, we can drop it and document or we can leave it and not document - my (paranoid) preference is to leave it to handle the case and not touch any docs.

@pdhamdhere
Copy link
Contributor

LGTM

@msterin msterin merged commit eaa2e33 into master Mar 7, 2017
@msterin msterin deleted the vers_check.msterin branch March 7, 2017 01:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants