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.

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

*: fix gen-csv to copy CRD manifests with any name #2015

Merged
merged 1 commit into from
Oct 7, 2019
Merged

*: fix gen-csv to copy CRD manifests with any name #2015

merged 1 commit into from
Oct 7, 2019

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Oct 7, 2019

Description of the change:
Relax _crd.yaml suffix check when copying over CRD manifests for operator-sdk olm-catalog gen-csv --update-crds

Motivation for the change:
Fixes #1980

@hasbro17 hasbro17 added the kind/bug Categorizes issue or PR as related to a bug. label Oct 7, 2019
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 7, 2019
@hasbro17 hasbro17 removed the request for review from AlexNPavel October 7, 2019 05:41
@hasbro17 hasbro17 force-pushed the bugfix-allow-all-names-in-gen-csv-config branch from 026df29 to 3245561 Compare October 7, 2019 05:42
@hasbro17 hasbro17 added the release-blocker This issue blocks the parent release milestone label Oct 7, 2019
@camilamacedo86
Copy link
Contributor

/lgtm
I am looking to test it locally to approve.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019
camilamacedo86
camilamacedo86 approved these changes Oct 7, 2019
View reviewed changes
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.

PR tested locally.

  • Create a CR without the prefix and checked that it was not copied
  • Create a CRD and checked that it was copied with success.

Worked 100% fine.

@camilamacedo86
Copy link
Contributor

WDYT about we track an issue for we are able to impl test for olm commands?

joelanford
joelanford approved these changes Oct 7, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

@camilamacedo86 camilamacedo86 merged commit 5d22a11 into operator-framework:master Oct 7, 2019
@hasbro17
Copy link
Contributor Author

hasbro17 commented Oct 7, 2019

@camilamacedo86 I know we have unit tests to test out the CSV scaffolding logic but I don't think we have e2e tests that tests out the path of the gen-csv command itself. @estroz might know more about that but yeah I think we should probably create an issue to track the need to test out the csv-gen command.

@hasbro17 hasbro17 deleted the bugfix-allow-all-names-in-gen-csv-config branch October 7, 2019 19:50
@camilamacedo86
Copy link
Contributor

Done : #2016

@hasbro17 hasbro17 mentioned this pull request Oct 7, 2019
Closed
@hasbro17 hasbro17 restored the bugfix-allow-all-names-in-gen-csv-config branch October 7, 2019 22:44
hasbro17 added a commit to hasbro17/operator-sdk that referenced this pull request Oct 8, 2019
@hasbro17
*: fix gen-csv to copy CRD manifests with any name (operator-framewor…
cd0711c 
…k#2015)

(cherry picked from commit 5d22a11)
This was referenced Oct 8, 2019
Merged
Merged
hasbro17 added a commit to hasbro17/operator-sdk that referenced this pull request Oct 9, 2019
@hasbro17
*: fix gen-csv to copy CRD manifests with any name (operator-framewor…
1ff2ed7 
…k#2015)

(cherry picked from commit 5d22a11)
hasbro17 added a commit that referenced this pull request Oct 9, 2019
@hasbro17
cherry-pick: csv-gen bugfixes for #2015 #2017 (#2021)
7052e8e 
* *: 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)
@hasbro17 hasbro17 deleted the bugfix-allow-all-names-in-gen-csv-config branch October 10, 2019 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@estroz estroz Awaiting requested review from estroz

Assignees

@camilamacedo86 camilamacedo86

Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-blocker This issue blocks the parent release milestone size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gen-csv requires CRD files to have _crd.yaml suffix
4 participants
@hasbro17 @camilamacedo86 @joelanford @openshift-ci-robot