Skip to content

Remove CSVConfig and use CSV Generator for configurable inputs and output #2511

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

Merged

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Feb 7, 2020

Description of the change:
Continuation of #2407 to implement the CSV generator.

  • cmd/operator-sdk/olmcatalog: remove config flag, add include flag, use CSV and package manifest generators

  • internal/generate/olm-catalog: implement CSV generator, moving most of the code from internal/scaffold/olm-catalog verbatim. Also removed CSV config

  • doc: remove CSVConfig documentation, update CLI docs

Currently this gets #2407 rebased with the master and breaks up the internal/generate/olm-catalog pkg changes, and the internal/generate/testdata/ manifests changes into separate commits for ease of reviewing.

TODO:

  • Change --include flag to --inputs to allow configuring the CSV generator Inputs field for Deploy and API directory paths e.g:
    --input deploy=./deploy/production/...,apis=./pkg/apis
  • Remove Filters field from generator config since its UX was ambiguous
  • Add flag --outputDir to set CSV Generator Output field to set the output root directory for the olm-catalog bundles directory.

Motivation for the change:

Resolves #2493

@hasbro17 hasbro17 added refactoring olm-integration Issue relates to the OLM integration labels Feb 7, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2020
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 7, 2020
@hasbro17 hasbro17 requested review from estroz and removed request for joelanford and theishshah February 7, 2020 07:14
@hasbro17
Copy link
Contributor Author

hasbro17 commented Feb 7, 2020

@estroz No need to review until I finish the TODOs but a quick sanity check over the first commit would be helpful to ensure I've carried forward the necessary changes from #2407

Had to untangle that PR a bit to rebase and split up the commits since quite a few changes from that had already gone into master.

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.

Looks like you got everything. I would remove the Helm tests from this PR and make a follow-up (that was my plan but I didn't get around to it).

@hasbro17
Copy link
Contributor Author

hasbro17 commented Feb 7, 2020

Yeah I'll remove the Helm tests from this. I initially split it up as a separate commit but a follow up would be easier to review.

@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from 20cf1e6 to 321663b Compare February 9, 2020 22:43
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2020
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from 321663b to 9869d11 Compare February 11, 2020 06:09
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from 9869d11 to d2eea3d Compare February 11, 2020 06:23
@hasbro17
Copy link
Contributor Author

@estroz I've done some manual tests and have this mostly working.
I still need to clean up a couple of things plus update the unit tests, but I've got a couple of questions that came up as I was refactoring.
I've marked them as TODO comments if you could take a look at those. Just the 2nd commit which has my changes to your old PR.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from eaec8a1 to 23be244 Compare February 12, 2020 08:26
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch 2 times, most recently from 3196016 to 3332819 Compare February 12, 2020 09:03
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from d2d2584 to 7186860 Compare March 27, 2020 21:13
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from 7186860 to ea6dd38 Compare March 27, 2020 23:56
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from ea6dd38 to 1721672 Compare March 31, 2020 16:58
hasbro17 and others added 14 commits March 31, 2020 11:32
internal/generate/olm-catalog: implement CSV and package manifest
generators, moving most of the code from internal/scaffold/olm-catalog
verbatim. Also removed CSV config

doc: remove CSVConfig documentation, update CLI docs

Co-authored-by: Eric Stroczynski <[email protected]>
bundleDirKey and manifestsDirKey were not suited to be generator input keys.
Much simpler to have them as private fields.
Plus some linter and cli docs cleanup.
…put dir configured

Also relax the requirement for running generate csv in project root
…with individual flags, add caveats and TODOs
Instead only skip when API types directory is missing.
@hasbro17 hasbro17 force-pushed the haseeb/feature/gen-csv-poc branch from 1721672 to 8774de6 Compare March 31, 2020 18:32
@hasbro17
Copy link
Contributor Author

hasbro17 commented Apr 1, 2020

Sigh
@estroz Any clue why the scorecard test for bundle validation might be failing? I'm not sure if after rebasing from master if the scorecard now relies on any testdata manifests that I've modified/moved around in this PR.
https://travis-ci.org/github/operator-framework/operator-sdk/jobs/669505119#L1244

olm:
	Bundle Validation Test              
	CR: example-memcached
	Labels: 
		"necessity":"required"
		"suite":"olm"
		"test":"bundlevalidationtest"
	Errors: 
		unable to find the OLM '\''bundle'\'' directory which is required for this test
	Provided APIs have validation       pass
	CR: example-memcached
	Labels: 
		"test":"crdshavevalidationtest"
		"necessity":"required"
		"suite":"olm"

Or if scorecard is using generate csv to generate the bundle directory and somehow failing to.
Going to dig into this a bit.

…2/memcached-operator.v0.0.2.clusterserviceversion.yaml
@hasbro17 hasbro17 merged commit 0e3eaf6 into operator-framework:master Apr 1, 2020
@hasbro17
Copy link
Contributor Author

hasbro17 commented Apr 1, 2020

@estroz Thanks for the fix 🙏
Had to reset CI a few times to get marker to play nice but all clear in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
olm-integration Issue relates to the OLM integration size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ApisDir to be configurable
6 participants