feat: support v1 CRD objects in OLM #1416
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
exdx
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
Are there any docs anywhere that should be updated to reflect that OLM supports |
exdx
force-pushed
the
feat/v1-crds
branch
4 times, most recently
from
8b63433 to
7e377cf
Compare
|
/approve Looks good! I think this refactor will help with csvless bundles. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, exdx 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 |
openshift-ci-robot
added
the
approved
|
/retest |
|
/hold |
openshift-ci-robot
added
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
| switch step.Status { | ||
| case v1alpha1.StepStatusPresent, v1alpha1.StepStatusCreated: | ||
| doStep := true | ||
| s, err := b.step(step.Status, step.Resource, step.Resource.Name) |
There was a problem hiding this comment.
I think this should take the whole step as a single argument.
There was a problem hiding this comment.
Agreed. I renamed the function to create so we have create(step) for clarity as well.
|
|
||
| // Stepper manages cluster interactions based on the step. | ||
| type Stepper interface { | ||
| Status() (v1alpha1.StepStatus, error) |
There was a problem hiding this comment.
The naming here is a little confusing. Usually I would expect ${VERB}er to $VERB.
| } | ||
|
|
||
| // step is a factory that creates StepperFuncs based on the Kind provided and the install plan step. | ||
| func (b *builder) step(stepStatus v1alpha1.StepStatus, resource v1alpha1.StepResource, name string) (Stepper, error) { |
There was a problem hiding this comment.
It seems like builder is really the Stepper because it's -- indirectly -- performing the step. It also needs to have knowledge of all the Stepper implementations.
What if:
- A
Stepperhas aStep(take *v1alpha1.Step) (taken bool, err error)method, which performs an in-place update of the given step -- returningtaken=trueif the Stepper cared to take a step. - Different
Stepperimplementations are made for specific resource types; e.g.CSV,CRD,CR, etc... - A
StepperChainStepperimplementation is made that calls a set of steppers in order untiltaken=trueor an error is encountered -- forwarding the results along, or if no step was taken, returnstaken=false.
I think something similar to this will let us view the stepper order and inclusion more easily; e.g.:
// On the Operator struct
o.stepper = StepperChain{
NewCRDStepper(...),
NewCSVStepper(...),
NewDefaultStepper(...),
}
// ...
// In the InstallPlan transition
taken, err := o.stepper.Step(take)
if err != nil {
// ...
}
if !taken {
// ...
}
// ...There was a problem hiding this comment.
or maybe a continue bool return would be more flexible than taken -- the semantics would follow the namesake
There was a problem hiding this comment.
I'm down to think about this more next week
openshift-ci-robot
added
the
needs-rebase
|
We've identified a potential issues with int -> float conversion (see helm/helm#5806 for an example) going from an unstructured object to a v1 CRD. Will file a bug and resolve following feature freeze. |
openshift-ci-robot
removed
the
needs-rebase
exdx
force-pushed
the
feat/v1-crds
branch
2 times, most recently
from
46fa1e8 to
992a0c9
Compare
|
I believe e2e should pass now, barring flakes. |
|
/retest |
Stepper interface. All CRDs will be converted and handled at the v1 APIVersion.
|
/test e2e-aws-olm |
|
@exdx: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
upstream tests are passing with one known failure (external image dependency issue). /lgtm |
Description of the change:
The goal of this PR is to enable the catalog operator to fulfill install plans on bundles that contain CRDs that are in the newer
apiextensions.k8s.io/v1group and version. The OLM operator will also need an informer based on this new version. However, we still need to support the olderv1beta1version of CRDs as well. Most of the code from the existing reconciliation ofv1beta1type CRDs is duplicated to support thev1version.Additional work will be needed on the registry side to enable users to generate bundles with the new
v1CRDs and validate them.The biggest change between
v1beta1andv1version CRDs are around the validation fields - inv1beta1the validation was optional since the structural schema was not required and turned off by default. Inv1the structural schema is required, and validation is required on each version of the supported resources. So the spec has changed around to reflect this between the two versions.Note: The
v1beta1version of the CRD api will be served at thev1endpoint based on the kubernetes api conventions. So performing aGETwith the v1 CRD client on a v1beta1 CRD on-cluster will return the v1 representation of that CRD.https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility
Motivation for the change:
v1beta1CRDs will be deprecated in kube as of 1.19Reviewer Checklist
/docs