cmd/.../gen-csv: use --include instead of CSV config

[estroz]

Description of the change:

• internal/scaffold/olm-catalog: replace CSVConfig with genutil.Config and update related code
• cmd/operator-sdk/olmcatalog: use generator Config in scaffolds, remove config flag, add --include flag
• doc: remove CSVConfig documentation, update CLI docs
• internal/generate/util: add Config type and helper

Motivation for the change: CSVConfig configures olm-catalog gen-csv to include data from certain files/dirs in the generation process. This config object may change significantly if we add/remove requirements for CSV generation in the future or users request more config fields. Instead of breaking config structure, the generator should look through all files in a default location to gather what data it needs to compile a CSV, including dirs/files passed in by --include=[list of paths]. --include is a generalization of specifying exact paths required, so we can get the same effect as a config with much less actual configuration. The default include path is deploy/.

Closes #1346, closes #2266

Relates to #2201

[camilamacedo86]

@estroz I loved it 👍 Much easier and intuitive to be used and low code complexity.
The code is great 🥇

Just a few nits

• missing the CHANGELOG.
• Could you add an example over how to populate it to make simpler for the users? Could you add in this example 2 paths to show what character should be used to split them? I think it would be great in the usage description of the command. In this way, they do not really need to check the docs. (Added suggestions)
• a test using the exclude option.

[camilamacedo86]

Hi @estroz,

IHMO this PR 100% great and make the process very easier for the users.
See that we have another feature request for this area in order to have a fag to pass the operator-name when it is used for a no-SDK operator.

So, because of this, I think that may be valid we add here in this pr a flag to pass the path as well when the operator is no-SDK one. E.g ( --path= which by default will be the SDK layout but if informed should use the dir passed as arg )

OR

Keep the config just for no-sdk operators?

c/c @joelanford

[aliok]

hi @estroz

This change makes things simpler when there's an operator that uses Operator-SDK.

I was trying to use operator-sdk olm-catalog gen-csv with this operator: https://github.com/knative/eventing-operator/

It is not an Operator-SDK based operator.
CRDs, roles and operator deployment are in config/ directory.
I actually liked the fact that we can generate CSV for these kind of projects as well, by specifying the roles path etc. in csv-config.yaml. That was my motivation for #2266.

What do you think?
Why don't we keep the ability to specify paths in csv-config.yaml and support both types of operators?
New feature, --exclude, could be still in. That could even be configured in csv-config.yaml.

I think keeping the configuration options (operator-path, role-paths, crd-cr-paths properties in csv-config.yaml) is necessary. There are a lot of operators out there, which are not based on Operator-SDK but they still want to be in OperatorHub. Making this change IMO would make things harder for maintainers of these operators.

Or, let's dump csv-config.yaml but create command line arguments for operator-path, role-paths and crd-cr-paths.

[aliok]

I would be happy to provide additional information, if you need anything.

[matzew]

Why don't we keep the ability to specify paths in csv-config.yaml and support both types of operators?
New feature, --exclude, could be still in. That could even be configured in csv-config.yaml.

I agree with @aliok - keeping the existing path based approach makes it much easier for operators written with different (opinionated) runtimes (like the knative ones), but still using part from THIS community to generate the YAMLs.

I'd greatly appreciate to not remove the config flag

[estroz]

@aliok @matzew thanks for your feedback. This is a breaking change so its great to hear community input.

Part of the reason I want to remove the config file is that upcoming scaffold changes will likely include some kind of top-level configuration, which should not be modified often after it is added. However files required for CSV generation may change following that addition, and continue to change as CSV format is solidified (it is still v1alpha1). Even if top-level config is not incorporated or is far off in our development plan, changing CSV format is still an issue.

The other part is CSV updates. Say I have an existing CSV my-operator.v0.0.1 that is outdated (ex. the project is up to v0.2.0 now). The project has some legacy manifests that must be included in this CSV. This means that I as the operator maintainer would have to update the CSV config just for this update process, then revert those changes. Alternatively I could keep a legacy config, but that adds file maintenance overhead and clutter.

Using a general CLI option for config fits both situations without many/any changes necessary going forward and therefore is more maintainable.

Therefore I suggest an alternative --inputs option (name subject to change) that takes a list of path patterns to include in the generation process. For example:

gen-csv --inputs config,deploy/prod/some_crd.yaml

would include all files in config/ and deploy/prod/some_crd.yaml.

@joelanford @camilamacedo86 @hasbro17 thoughts?

[camilamacedo86]

IHMO:

• +1 for as @estroz described we increment this with flags and may we will be able to indeed allow customizations via the makefile in the future.
• This change increases the maintenance ability and makes easier its usage for SDK projects.

Also, note that no-SDK projects users still able to use it for their projects. They just need to move a few for the same SDK layout structure until we are able to provide the future impls/options.

For me, it is /lgtm /approve

[estroz]

Going to hold this until I write up a proposal describing why CLI config is better than a file, and get feedback.

/hold

[aliok]

@estroz thanks very much for the long explanation.

Therefore I suggest an alternative --inputs option (name subject to change) that takes a list of path patterns to include in the generation process.

That would be fine! Important bit I thing is the ability to define the inputs.

As per @camilamacedo86 's comment:

Also, note that no-SDK projects users still able to use it for their projects. They just need to move a few for the same SDK layout structure until we are able to provide the future impls/options.

Changing the layout of the Knative eventing operator for example, is not easy. We don't own that operator and I think there would be lots of objections if we want to move things.
However, as long as I can feed the files I want, and only the files I want to the CLI, it would work fine.
So, @estroz 's suggestion to have --inputs would work.

Overall, please keep in mind that projects might have different layouts and ability to override configuration would be very beneficial.

Thanks for your efforts folks!

[aliok]

Looks great @estroz

I can give it a try tomorrow.

[aliok]

hi @estroz

I gave it a try.

My include path doesn't have "deploy/crds" but I receive this error:

$ operator-sdk olm-catalog gen-csv            \
  --include       "config/operator.yaml,config/role.yaml,config/300-eventing-v1alpha1-knativeeventing-crd.yaml,config/role_binding.yaml,config/service_account.yaml" \
  --from-version  "0.10.1"       \
  --csv-version   "0.10.2"      \
  --operator-name knative-eventing-operator \
  --update-crds                             \
  --csv-channel   alpha                     \
  --default-channel
INFO[0000] Generating CSV manifest version 0.10.2       
INFO[0000] API directory pkg/apis/operator/v1alpha1 does not exist. Skipping CSV annotation parsing for API operator.knative.dev/v1alpha1, Kind=Eventing. 
INFO[0000] Skipping non-object manifest deploy/olm-catalog/knative-eventing-operator/knative-eventing-operator.package.yaml 
INFO[0000] Created deploy/olm-catalog/knative-eventing-operator/0.10.2/knative-eventing-operator.v0.10.2.clusterserviceversion.yaml 
INFO[0000] Created deploy/olm-catalog/knative-eventing-operator/knative-eventing-operator.package.yaml 
Error: lstat deploy/crds: no such file or directory
Usage:
  operator-sdk olm-catalog gen-csv [flags]

Flags:
      --csv-channel string     Channel the CSV should be registered under in the package manifest
      --csv-version string     Semantic version of the CSV
      --default-channel        Use the channel passed to --csv-channel as the package manifests' default channel. Only valid when --csv-channel is set
      --from-version string    Semantic version of an existing CSV to use as a base
  -h, --help                   help for gen-csv
      --include strings        Paths to include in CSV generation, ex. "deploy/prod,deploy/test". If this flag is set and you want to enable default behavior, you must include "deploy/" in the argument list (default [deploy])
      --operator-name string   Operator name to use while generating CSV
      --update-crds            Update CRD manifests in deploy/{operator-name}/{csv-version} the using latest API's

Global Flags:
      --verbose   Enable verbose logging

When I remove --update-crds, the error is gone.
So the command is trying to copy the crds from deploy/crds.

I hope we won't need a new argument for the program.
UPDATE: I think we would need it because configuration option for the crd paths were available in the csv-config.yaml file.
Can we also have --csv-path argument?

[estroz]

@aliok there's still some work that needs doing on this. Working on it now.

I'd like to keep the number of flags to a minimum. Perhaps just having --inputs is not enough though.

[openshift-ci-robot]

@estroz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[estroz]

Upon looking into removing the config with an --include option and non-default paths, it seems like rewriting the CSV generator internally (something we've had planned for a bit) will need to be done in tandem with config removal. It will require much less refactoring later if we do both simultaneously, since configuration of the refactored generator will be simpler.

For now I will fix the config bug in a separate PR and place a hold on this PR.

/hold

[aliok]

@estroz
Ok, I understand.
I will try to watch this PR and try to help with verification of the future modifications if possible.

[estroz]

Closing per #2249 (comment). Config removal will come when the CSV generator is updated before rewriting operator-sdk in kubebuilder plugin format.