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

Replace the controller manager's RESTMapper with a dynamic one #95

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

danwinship
Copy link
Contributor

controller-manager assumes that all resource Kinds will have been created before it starts up, so it can't deal with the case of creating a CRD and then creating an instance of that CRD (or waiting for another operator to create a CRD, and then creating an instance of that CRD). This is maybe not the bestest fix, but it works and we can discuss it further upstream.

Note that this code would be especially bad/inefficient if we thought that the controller was going to be repeatedly asked about objects of non-existent types, but there's no reason that would happen in CNO, so the reload path should only get hit in the creating-an-instance-of-a-previously-created-CRD case.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2019
@squeed
Copy link
Contributor

squeed commented Feb 11, 2019

/lgtm
Thanks for tracking that one down.
FYI: @shawn-hurley, you might be interested in this. Perhaps this should be the default for the operator-sdk

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2019
@JacobTanenbaum
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, JacobTanenbaum, squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 927708a into openshift:master Feb 11, 2019
@danwinship danwinship deleted the restmapper branch March 18, 2019 15:52
jcrossley3 added a commit to openshift-knative/knative-serving-operator that referenced this pull request Apr 15, 2019
ironcladlou added a commit to ironcladlou/cluster-dns-operator that referenced this pull request Jun 10, 2019
* Delete custom cache setup, no longer necessary
* Consolidate client usage and use dynamic discovery (see kubernetes-sigs/controller-runtime#321 — hat tip to @danwinship for openshift/cluster-network-operator#95). Fixes [bz1711373](https://bugzilla.redhat.com/show_bug.cgi?id=1711373).
* Plumb the cache through for future use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants