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

internal/pkg/scaffold/olm-catalog/csv_updaters.go: fix applying owned CRDs #2017

Merged

Conversation

joelanford
Copy link
Member

Description of the change:
This PR fixes an issue in olm-catalog gen-csv where the CSV's owned CRDs are not properly populated from CRD manifests in deploy/crds.

The current code has two issues:

  1. For new CSVs (when --from-version is not used), no CRDs are populated.
  2. For CSV updates (when --from-version is used), only CRDs that are in the original version will be carried forward to the new version, even if there are new CRDs present in deploy/crds

These issues stem from the fact that the CSV updater iterates the existing set of CRDs to build the new set. In case 1, the existing set is empty, since nothing exists, which explains why no CRDs are populated.

Motivation for the change:
Fixes the above described issue.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 7, 2019
@joelanford joelanford requested review from camilamacedo86 and hasbro17 and removed request for AlexNPavel October 7, 2019 20:45
@joelanford joelanford added the release-blocker This issue blocks the parent release milestone label Oct 7, 2019
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

Can you also add a line to the CHANGELOG for this fix.

@hasbro17
Copy link
Contributor

hasbro17 commented Oct 7, 2019

/test e2e-aws-ansible
/test e2e-aws-go

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2019
@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Oct 8, 2019

/test e2e-aws-ansible
/test e2e-aws-helm

@joelanford
Copy link
Member Author

/test e2e-aws-helm

@jmrodri jmrodri merged commit 5bfe311 into operator-framework:master Oct 8, 2019
@joelanford joelanford deleted the fix-gen-csv-owned-crds branch October 8, 2019 18:36
hasbro17 pushed a commit to hasbro17/operator-sdk that referenced this pull request Oct 8, 2019
… CRDs (operator-framework#2017)

* internal/pkg/scaffold/olm-catalog/csv_updaters.go: fix applying owned CRDs
* CHANGELOG.md: add line for operator-framework#2017

(cherry picked from commit 5bfe311)
hasbro17 pushed a commit to hasbro17/operator-sdk that referenced this pull request Oct 9, 2019
… CRDs (operator-framework#2017)

* internal/pkg/scaffold/olm-catalog/csv_updaters.go: fix applying owned CRDs
* CHANGELOG.md: add line for operator-framework#2017

(cherry picked from commit 5bfe311)
hasbro17 added a commit that referenced this pull request Oct 9, 2019
* *: fix gen-csv to copy CRD manifests with any name (#2015)


(cherry picked from commit 5d22a11)

* internal/pkg/scaffold/olm-catalog/csv_updaters.go: fix applying owned CRDs (#2017)

* internal/pkg/scaffold/olm-catalog/csv_updaters.go: fix applying owned CRDs
* CHANGELOG.md: add line for #2017

(cherry picked from commit 5bfe311)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. release-blocker This issue blocks the parent release milestone size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants