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

Allow templateinstance controller to instantiate non-v1 objects #14799

Merged
merged 1 commit into from
Jun 24, 2017

Conversation

jim-minter
Copy link
Contributor

fixes #14780

@jim-minter jim-minter self-assigned this Jun 21, 2017
@jim-minter
Copy link
Contributor Author

jim-minter commented Jun 21, 2017

  • implement test case

@jim-minter
Copy link
Contributor Author

@deads2k @soltysh please can you take a look at this? It seems to solve the issue raised, but:

  1. I've copied code from https://github.com/openshift/origin/blob/master/pkg/build/admission/jenkinsbootstrapper/admission.go#L107-L133 , which is in itself naughty
  2. https://github.com/openshift/origin/blob/master/pkg/project/registry/projectrequest/delegated/delegated.go#L174 is also doing this wrong

I think a helper function like https://github.com/openshift/origin/blob/master/pkg/cmd/util/clientcmd/factory.go#L105 would be nice, but I don't know how to create all the clientcmd scaffolding in the contexts above (or better, how to avoid creating all the clientcmd scaffolding).

Please can you accept or suggest an acceptable achievable refactor?

@bparees

@soltysh
Copy link
Contributor

soltysh commented Jun 22, 2017

I'm fine with this copy&paste for now. We'll probably want to address that in 3.7, properly. The only missing thing here is a test-case proving that different k8s resources can be easily created. @deads2k agreed?

@@ -307,10 +311,35 @@ func (c *TemplateInstanceController) instantiate(templateInstance *templateapi.T
RESTMapper: client.DefaultMultiRESTMapper(),
ObjectTyper: kapi.Scheme,
ClientMapper: resource.ClientMapperFunc(func(mapping *meta.RESTMapping) (resource.RESTClient, error) {
config := *c.config

// TODO copied from pkg/cmd/util/clientcmd/factory_object_mapping.go#ClientForMapping
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you move this to an openshift /resource package that returns a clientmapperfunc based on a config?

@deads2k
Copy link
Contributor

deads2k commented Jun 22, 2017

Please can you accept or suggest an acceptable achievable refactor?

Suggestion made. I think that centralizing the problem and creating a clientmapperfunc from a config would be sufficient to move us forward.

@jim-minter
Copy link
Contributor Author

updated.

[test][testextended][extended:core(templates)]

@jim-minter jim-minter assigned bparees and unassigned jim-minter Jun 22, 2017
@jim-minter jim-minter changed the title WIP allow templateinstance controller to instantiate non-v1 objects Allow templateinstance controller to instantiate non-v1 objects Jun 22, 2017
@bparees
Copy link
Contributor

bparees commented Jun 22, 2017

removed the merge tag, this appears to have panicked our extended tests:
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/691/testReport/junit/(root)/Extended/BeforeSuite/

haven't looked into it to confirm it is the culprit.

@jim-minter
Copy link
Contributor Author

oops, too many threads in parallel - fixed and repushed

@openshift openshift deleted a comment from openshift-bot Jun 22, 2017
@bparees
Copy link
Contributor

bparees commented Jun 22, 2017

@@ -7319,7 +7319,7 @@ items:
         metadata:
           name: secret
       - kind: Deployment
-        apiVersion: extensions/v1beta1
+        apiVersion: apps/v1beta1
         metadata:
           name: deployment
         spec:
FAILURE: Generated test/extended bindata out of date. Please run hack/update-generated-bindata.sh

@jim-minter
Copy link
Contributor Author

fixed and repushed

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to 2aabf5d

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 2aabf5d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/711/) (Base Commit: c3c286b) (PR Branch Commit: 2aabf5d) (Extended Tests: core(templates))

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2544/) (Base Commit: c3c286b) (PR Branch Commit: 2aabf5d)

@soltysh
Copy link
Contributor

soltysh commented Jun 23, 2017

LGTM

@bparees
Copy link
Contributor

bparees commented Jun 23, 2017

[merge][severity:blocker]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 2aabf5d

@openshift openshift deleted a comment from openshift-bot Jun 24, 2017
@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 24, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1115/) (Base Commit: 9b2743f) (PR Branch Commit: 2aabf5d) (Extended Tests: blocker) (Image: devenv-rhel7_6396)

@openshift-bot openshift-bot merged commit 1e19d81 into openshift:master Jun 24, 2017
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.

Template broker can't provision a template that contains a Deployment resource
5 participants