-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Remove CSVConfig and use CSV Generator for configurable inputs and output #2511
Conversation
@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. |
There was a problem hiding this 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).
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. |
20cf1e6
to
321663b
Compare
321663b
to
9869d11
Compare
9869d11
to
d2eea3d
Compare
@estroz I've done some manual tests and have this mostly working. |
eaec8a1
to
23be244
Compare
3196016
to
3332819
Compare
d2d2584
to
7186860
Compare
New changes are detected. LGTM label has been removed. |
7186860
to
ea6dd38
Compare
ea6dd38
to
1721672
Compare
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.
1721672
to
8774de6
Compare
Sigh 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. |
...oy/olm-catalog/memcached-operator/0.0.2/memcached-operator.v0.0.2.clusterserviceversion.yaml
Show resolved
Hide resolved
…2/memcached-operator.v0.0.2.clusterserviceversion.yaml
@estroz Thanks for the fix 🙏 |
Description of the change:
Continuation of #2407 to implement the CSV generator.
Currently this gets #2407 rebased with the master and breaks up the
internal/generate/olm-catalog
pkg changes, and theinternal/generate/testdata/
manifests changes into separate commits for ease of reviewing.TODO:
--include
flag to--inputs
to allow configuring the CSV generatorInputs
field for Deploy and API directory paths e.g:--input deploy=./deploy/production/...,apis=./pkg/apis
Filters
field from generator config since its UX was ambiguous--outputDir
to set CSV GeneratorOutput
field to set the output root directory for theolm-catalog
bundles directory.Motivation for the change:
Resolves #2493