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

Adding support for installing Kubernetes 1.8.4 #311

Merged
merged 2 commits into from
Dec 7, 2017

Conversation

amccormi
Copy link

Fixes-Jira: CNTV-100

@@ -32,6 +32,7 @@ k8s_ver = ENV['CONTIV_K8S_VERSION'] || DEFAULT_K8S_VERSION
orc_path = case k8s_ver
when /^v1\.[45]\./ then 'k8s1.4/'
when /^v1\.[67]\./ then 'k8s1.6/'
when /^v1\.[89]\./ then 'k8s1.8/'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably just have this match 1.8 for now... otherwise people might try 1.9 (and we'll allow them to!) before we've verified that 1.9 actually works 😉

Copy link
Author

Choose a reason for hiding this comment

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

Good point :)

@@ -4,7 +4,7 @@ require 'rubygems'
require 'fileutils'
require 'yaml'

DEFAULT_K8S_VERSION = 'v1.6.5'.freeze
DEFAULT_K8S_VERSION = 'v1.8.2'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1.8.2 instead of 1.8.4 (latest)?

Copy link
Author

Choose a reason for hiding this comment

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

Will test with 1.8.4

@dseevr dseevr changed the title Adding support for installing Kubernetes 1.8.2 Adding support for installing Kubernetes 1.8.4 Nov 30, 2017
@@ -0,0 +1,61 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a new copy of the yaml files if they are an exact copy of 1.6 folder?

Copy link
Author

Choose a reason for hiding this comment

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

While it's true that the files in install/k8s/k8s1.6 and k8s1.8 are the same, the files in cluster/k8s/k8s1.6 and k8s1.8 are not the same, so it seems weird to me to in one case point to 1.6 files for 1.8, and in another case, point to 1.8 files.

Copy link
Member

Choose a reason for hiding this comment

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

There maybe additional change for 1.8 testing. I think it is cleaner to have a separated folder for 1.8.

@@ -17,8 +17,10 @@ fi
k8sversion=$($kubectl version --short | grep "Server Version")
if [[ "$k8sversion" == *"v1.4"* ]] || [[ "$k8sversion" == *"v1.5"* ]]; then
k8sfolder="k8s1.4"
else
elif [[ "$k8sversion" == *"v1.6"* ]] || [[ "$k8sversion" == *"v1.7"* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the same folder for 1.8 as well, if required rename it to 1.8?

Copy link
Author

Choose a reason for hiding this comment

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

While it's true that the files in install/k8s/k8s1.6 and k8s1.8 are the same, the files in cluster/k8s/k8s1.6 and k8s1.8 are not the same, so it seems weird to me to in one case point to 1.6 files for 1.8, and in another case, point to 1.8 files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to rename install/k8s/k8s1.6 to install/k8s/rbac and install/k8s/k8s1.4 to install/k8s/pre-rbac? The primary difference between the two versions is that RBAC was added post 1.4. Cluster scripts install a specific version and are more version specific rather that being related to a featureset like the yaml files.

Copy link
Author

Choose a reason for hiding this comment

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

It's an interesting idea, but I'm envisioning what that would look like with the next big feature set. In the future, we'd add another feature, create a new directory, but it won't be obvious that rbac is included with the new feature. It won't be obvious that it is cumulative, in other words. Would it be better to make the k8s1.8 directory a sim link to k8s1.6?

Copy link
Author

Choose a reason for hiding this comment

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

Talked with Neelima. Here is the plan: we'll leave the k8s1.4 directory intact, as it will go away soonish. The k8s1.6 directory will be renamed rbac, and 1.6, 1.7 and 1.8 will use this directory.

Copy link
Contributor

@neelimamukiri neelimamukiri 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 making the changes.

@neelimamukiri neelimamukiri merged commit f0066e7 into contiv:master Dec 7, 2017
@amccormi amccormi deleted the CNTV-100 branch December 19, 2017 16:36
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.

4 participants