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

cluster up: prevent admin templates from instantiating on older versions #14362

Merged
merged 1 commit into from
May 27, 2017

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented May 25, 2017

Fixes #14357

@csrwng
Copy link
Contributor Author

csrwng commented May 25, 2017

@openshift/devex ptal
/cc @jcantrill @spadgett

}

func shouldImportAdminTemplates(v semver.Version) bool {
return v.GT(openshiftVersion35)
Copy link
Contributor

Choose a reason for hiding this comment

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

this means we'll install them on 3.5.1, right? is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right ... no, it needs to be 3.6 ... will change the condition to >= 3.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bparees bparees self-assigned this May 25, 2017
@csrwng csrwng force-pushed the clusterup_admin_templates branch from 32a55cf to 8048bc2 Compare May 25, 2017 19:17
@bparees
Copy link
Contributor

bparees commented May 25, 2017

lgtm

@csrwng csrwng force-pushed the clusterup_admin_templates branch from 8048bc2 to 1342989 Compare May 25, 2017 19:52
@csrwng
Copy link
Contributor Author

csrwng commented May 25, 2017

@bparees I fixed the error check in the ServerVersion method, it was backwards ... should cache the result if there is no error. If it still looks good to you is it ok to merge?

@bparees
Copy link
Contributor

bparees commented May 25, 2017

ah, i thought that was intentional that you were clearing part of the version if you got an error. so much for my faith in you :)

@bparees
Copy link
Contributor

bparees commented May 25, 2017

[merge][severity:bug]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 1342989

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 1342989

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1781/) (Base Commit: d03a427)

@csrwng
Copy link
Contributor Author

csrwng commented May 26, 2017

Flake #13980

Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

I think I can steal some of the 3.6.0 code check you have here for my use ansible evaluation. LGTM

@openshift-bot
Copy link
Contributor

openshift-bot commented May 27, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/821/) (Base Commit: 14f3300) (Extended Tests: bug) (Image: devenv-rhel7_6280)

@openshift-bot openshift-bot merged commit b784ef0 into openshift:master May 27, 2017
@smarterclayton
Copy link
Contributor

Why does template creation fail the cluster up? We should just warn ("couldn't add template X: reason")

@smarterclayton
Copy link
Contributor

This isn't really versioning - this is that people are writing templates that aren't compatible on older versions because of api groups. That's a much more fundamental problem.

@csrwng
Copy link
Contributor Author

csrwng commented Jun 6, 2017

@smarterclayton the problem with simply warning about the template creation error is that we'll have a partially imported template... is that ok?

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.

5 participants