Skip to content

Conversation

estroz
Copy link
Member

@estroz estroz commented May 5, 2020

CRDs should not be compared by name because each CSV customresourcedefinition element is not unique by name only, i.e. multiple versions of the same CRD are allowed.

Also fixed a test comparison (wrong argument order).

/kind bug

/cc @dinhxuanvu @kevinrizza

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 5, 2020
@dinhxuanvu
Copy link
Member

Looking good but hold for now.
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2020
@dinhxuanvu
Copy link
Member

/lgtm
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
if len(crd.Spec.Versions) == 0 {
// Skip group, which CSVs do not support.
key := registry.DefinitionKey{
Name: crd.GetName(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Our work adding v1 crds to bundles is going to break this - instead of returning a *v1beta1.CustomResourceDefinition we are returning an interface now. We started addressing this in https://github.com/operator-framework/api/pull/29/files#diff-8a818e661938b6d25ef2013d1289f89cR94
but I believe we are planning to move this code back into the registry instead - right @dinhxuanvu?

See operator-framework/operator-registry#295 for the PR with the breaking change. we are planning to merge this one this week.

We can merge as-is and then fix it in a follow-up, or hold off on this change.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is meant to fix api as it is. We will address the v1 CRD support in subsequent PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds good

@exdx
Copy link
Contributor

exdx commented May 5, 2020

/lgtm

@dinhxuanvu
Copy link
Member

/lgtm

@dinhxuanvu dinhxuanvu merged commit 5ad863a into operator-framework:master May 5, 2020
@estroz estroz deleted the bugfix/crd-cmp-by-key branch May 21, 2020 14:11
kevinrizza added a commit that referenced this pull request Jun 2, 2020
validation: bundle CRDs are compared by definition key instead of name (#32)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants