-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Update required docker version to 1.12, including oc cluster up #13016
Conversation
[test] |
defer to @csrwng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in general, LGTM, just a minor nit.
This means that even though you specify an older version of openshift with the --version parameter (which could presumably run on an older Docker), if you have a newer oc binary, we won't let you.
@bparees @jim-minter if you're ok with that, I'm ok with that.
propagationMode = ":rslave" | ||
} | ||
binds = append(binds, fmt.Sprintf("%[1]s:%[1]s%[2]s", opt.HostVolumesDir, propagationMode)) | ||
binds = append(binds, fmt.Sprintf("%[1]s:%[1]s%[2]s", opt.HostVolumesDir, ":rslave")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit, since the propagation mode is now fixed, you can just make it part of the format string.
how do we feel about making it a warning instead of an error? |
I'm fine with that, but a warning is likely to go unnoticed until we fix #12715 |
bump |
@bparees @jim-minter not sure how useful this check is going to be going forward given the new versioning scheme: |
@csrwng bumped - now a warning rather than an error. I think this check is better than nothing - as things stand if the user is using a CE version number, we don't warn and that's fine (logically it should work as the codebase is newer than 1.12). If we see they are running < 1.12, we warn. |
Evaluated for origin test up to 7e5c173 |
continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/911/) (Base Commit: 192e57e) |
LGTM [merge] |
Evaluated for origin merge up to 7e5c173 |
continuous-integration/openshift-jenkins/test SUCCESS |
@@ -9,7 +9,7 @@ | |||
%global kube_plugin_path /usr/libexec/kubernetes/kubelet-plugins/net/exec/redhat~openshift-ovs-subnet | |||
|
|||
# docker_version is the version of docker requires by packages | |||
%global docker_version 1.9.1 | |||
%global docker_version 1.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdawson is this change already in OCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not in 3.5 or 3.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is OK for this to merge in for 3.5? Do we need to backport it?
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/2/) (Base Commit: 21527d7) (Image: devenv-rhel7_6056) |
No description provided.