Skip to content

fix(crd): Fix CRD validation falsely error on unused CRD versions #44

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 23, 2020
Merged

fix(crd): Fix CRD validation falsely error on unused CRD versions #44

merged 1 commit into from
Jun 23, 2020

Conversation

dinhxuanvu
Copy link
Member

CRD can have multiple versions and CSV can only reference one version.
CRD validation falsely errors out when there are unused versions in
provided CRDs.

Signed-off-by: Vu Dinh vdinh@redhat.com

@dinhxuanvu
fix(crd): Fix CRD validation falsely error on unused CRD versions
bedd1e3 
CRD can have multiple versions and CSV can only reference one version.
CRD validation falsely errors out when there are unused versions in
provided CRDs.

Signed-off-by: Vu Dinh <vdinh@redhat.com>
@estroz
Copy link
Member

estroz commented Jun 22, 2020

@dinhxuanvu will OLM ever install a non-stored version of an owned CRD? Can the validator only check stored: true CRD versions?

@dinhxuanvu
Copy link
Member Author

@dinhxuanvu will OLM ever install a non-stored version of an owned CRD? Can the validator only check stored: true CRD versions?

I'm not sure what you mean by installing a version within a CRD. OLM installs the entire CRD and it is up to operator to use which version of CRD. The owned CRD section in CSV is to ensure we know which version in the CRD that this operator intends to use/control. There can be multiple served versions that can be used by multiple different operators.

@dinhxuanvu
Copy link
Member Author

@estroz Can I have a lgtm from you so I can merge this PR?

estroz
estroz approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

This works although it would be simpler to reverse the order of comparison to check if a CRD version is an owned CRD instead of if an owned CRD has a corresponding CRD version. Something for a follow-up.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
@dinhxuanvu
Copy link
Member Author

This works although it would be simpler to reverse the order of comparison to check if a CRD version is an owned CRD instead of if an owned CRD has a corresponding CRD version. Something for a follow-up.

/lgtm

We still need to alert errors when a CRD is included in bundle and not present in CSV and vice versa so I don't think the order matters here.

@dinhxuanvu dinhxuanvu merged commit 9f0bd9c into operator-framework:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@gallettilance gallettilance gallettilance approved these changes

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

Assignees

@estroz estroz

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dinhxuanvu @estroz @gallettilance @openshift-ci-robot