internal/generator/olm-catalog: implement CSV generator #2407

estroz · 2020-01-14T23:19:45Z

Description of the change:

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

Motivation for the change: prepare CSV generator for plugin refactor. CSV config was removed in favor of the more flexible --include flag.

use CSV and package manifest generators 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

estroz · 2020-01-22T17:47:22Z

@aliok you can test #2279 using this branch. Note --inputs -> --include.

aliok · 2020-01-23T09:36:38Z

hi @estroz
Thanks a lot for the work. I think we're getting closer to an ideal functionality.

I found these problems:

1. Need `build/Dockerfile`.

The operator I am trying to run the CLI against is not using Operator SDK and no Dockerfile exists. We use ko to build the images.
This wasn't happening in my trials with previous versions of the tool.

2. Execution of command won't copy the CRD file.

My execution:

GO111MODULE="on" operator-sdk generate 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.11.0"       \
  --csv-version   "0.12.0"       \
  --operator-name knative-eventing-operator \
  --csv-channel   alpha                     \
  --default-channel                         \
  --output-dir    "deploy/olm-catalog/knative-eventing-operator/0.12.0"      \
  --verbose

Tree before:

[aliok@localhost eventing-operator]$ tree deploy/
deploy/
└── olm-catalog
    └── knative-eventing-operator
        ├── 0.11.0
        │   ├── eventing_v1alpha1_knativeeventing_crd.yaml
        │   └── knative-eventing-operator.v0.11.0.clusterserviceversion.yaml
        └── knative-eventing-operator.package.yaml

Tree after execution:

[aliok@localhost eventing-operator]$ tree deploy/
deploy/
└── olm-catalog
    └── knative-eventing-operator
        ├── 0.11.0
        │   ├── eventing_v1alpha1_knativeeventing_crd.yaml
        │   └── knative-eventing-operator.v0.11.0.clusterserviceversion.yaml
        ├── 0.12.0
        │   ├── knative-eventing-operator.package.yaml
        │   └── knative-eventing-operator.v0.12.0.clusterserviceversion.yaml
        └── knative-eventing-operator.package.yaml

The CRD file is included in the --include, but wont' be outputted to directory for 0.12.0 (the new CSV version).
If I add --update-crds, I receive this message:

FATA[0000] open deploy/olm-catalog/knative-eventing-operator/0.12.0/knative-eventing-operator/0.12.0/config/300-eventing-v1alpha1-knativeeventing-crd.yaml: no such file or directory

I guess the message above is normal, as --update-crds tries to update and overwrite CRD file in olm-catalog. We need some mechanism to also copy the crd into new directory for new CSV version.

3. Package and CSV is put into the same directory.

You can see that in the trees above.
If I use --output-dir "deploy/olm-catalog/knative-eventing-operator/", this time the package will be placed correctly but no nested directory will be created for the new CSV version.

Sorry for complaining a lot :( I am simply trying to make operator-sdk more useful in non-Operator-SDK scenarios in hope for more adoption in all operator communities.

4. Need `GO111MODULE="on"`

Need GO111MODULE="on" when running operator-sdk CLI. The operator I am trying to run the CLI against is not using Go modules. Adding GO111MODULE="on" won't make any harm when running the gen csv command, but it is annoying and maybe a sign of something wrong.
I am not sure if this is related to your changes in this PR. I guess probably not.
This is not a major issue.

estroz · 2020-01-23T20:55:08Z

@aliok thanks a bunch for testing this.

Turns out that exposing output-dir adds unnecessary complexity and results in potentially incorrect assumptions (resulting in 2 and 3 above) in the CSV generator, so I've removed it. It wasn't useful either, since CSV's should be read from a standard location for --from-version to work.

1 was introduced in #2322 to uphold assumptions the generator makes about where it is run. Perhaps we can relax this requirement to just having a deploy directory present.

I was not able to reproduce 4. Can you post your project's path? Does this affect our latest release (v0.15.0)?

aliok · 2020-01-24T09:22:51Z

@estroz

I see (2) and (3) are now fixed. That's really awesome!

1 was introduced in #2322 to uphold assumptions the generator makes about where it is run. Perhaps we can relax this requirement to just having a deploy directory present.

That would be great I think.

I was not able to reproduce 4. Can you post your project's path? Does this affect our latest release (v0.15.0)?

It happens with 0.15.0 as well. I think that's probably a problem on my side.

The project is at standard GOPATH (/home/aliok/go/src/knative.dev/eventing-operator) and is not a GoModules project (uses dep).
My GO111MODULE is off in my env vars.
I guess we should probably ignore this problem.

Thanks a lot again. Once we relax the rule for (1), we will be able to use the operator-sdk CLI with no problems at the Knative upstream operators.

hasbro17 · 2020-01-27T18:34:00Z

cmd/operator-sdk/generate/csv.go

-		err = writeCRDsToDir(csvCfg.CRDCRPaths, filepath.Dir(input.Path))
-		if err != nil {
-			return err
+		bundleDir := filepath.Join(gencatalog.OLMCatalogDir, strings.ToLower(c.operatorName), c.csvVersion)


@estroz I might have missed this from previous PRs but if the CSV generator inputs are meant to be configurable i.e change g.inputs[DeployDirKey] to point to another location deploy/production/ or in my case config for Kubebuilder's layout, then updating the CRD manifest to that new location would fail.
While the CSV and package manifests are written out to the new location, OLMCatalogDir is currently fixed to always be rooted at the deploy/ directory so it will try to update the CRDs there.

Is there any way we can stop using that constant, and instead use the generator's input for DeployDirKey?

Not sure if any other cmds that need to have a configurable "deploy" directory are using this constant as well. That could be another thing to watch out for.

Going to reconsider how inputs/filters are set up.

camilamacedo86 · 2020-02-04T01:08:52Z

internal/util/k8sutil/crd.go

+			if typeMeta, err := GetTypeMetaFromBytes(b); err == nil {
+				if typeMeta.Kind == "CustomResourceDefinition" {
+					crdPaths = append(crdPaths, path)
+				}


camilamacedo86

Hi @estroz,

It shows great. Just missing a rebase.

camilamacedo86 · 2020-02-04T01:15:07Z

CHANGELOG.md

@@ -18,6 +19,8 @@

 ### Removed

+- **Breaking Change:** Removed CSV configuration file support in favor of including files as input to the generator using [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options), defaulting to the `deploy/` directory. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))


Suggested change

- **Breaking Change:** Removed CSV configuration file support in favor of including files as input to the generator using [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options), defaulting to the `deploy/` directory. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))

- **Breaking Change:** Removed CSV configuration file support (Defaults to deploy/olm-catalog/csv-config.yaml) in favor of including files as input to the generator using [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options), defaulting to the `deploy/` directory. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))

hasbro17 · 2020-02-04T19:41:27Z

@camilamacedo86 We discussed some necessary changes to the flags and generator config to better facilitate inputs/output for non-standard projects layout.
I'll be taking over this PR to add those in, along with the rebase.

openshift-ci-robot · 2020-02-04T19:41:34Z

@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.

hasbro17 · 2020-02-04T19:42:12Z

/hold

aliok · 2020-02-05T07:46:04Z

@hasbro17 let me know if you need somebody to give things a try

hasbro17 · 2020-02-07T07:23:18Z

Closing this in favor of #2511 although that's still a WIP so not quite ready for reviews yet.

estroz added 3 commits January 9, 2020 12:57

Merge branch 'master' into feature/gen-csv-poc

19f3eae

fix up code after merge

bd53fde

estroz added olm-integration Issue relates to the OLM integration kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form labels Jan 14, 2020

estroz requested review from jmrodri, hasbro17, theishshah and camilamacedo86 January 14, 2020 23:19

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 14, 2020

estroz added 7 commits January 14, 2020 16:00

remove unused code

88910c1

Merge branch 'master' into feature/gen-csv-poc

7ca8662

doc/cli: regenerate

f4778df

fix order that inputs are set in CSV and package manifest generators

6fab2c9

clean up a few files from merge

8679db1

fix CHANGELOG.md

ba3bfa6

Merge branch 'master' into feature/gen-csv-poc

b46264e

remove output-dir flag

5f3699c

hasbro17 reviewed Jan 27, 2020

View reviewed changes

hasbro17 mentioned this pull request Feb 3, 2020

Allow ApisDir to be configurable #2493

Closed

camilamacedo86 reviewed Feb 4, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2020

hasbro17 mentioned this pull request Feb 7, 2020

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

Merged

hasbro17 closed this Feb 7, 2020

estroz deleted the feature/gen-csv-poc branch April 1, 2020 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

internal/generator/olm-catalog: implement CSV generator #2407

internal/generator/olm-catalog: implement CSV generator #2407

Uh oh!

estroz commented Jan 14, 2020

Uh oh!

estroz commented Jan 22, 2020

Uh oh!

aliok commented Jan 23, 2020 •

edited

Loading

Uh oh!

estroz commented Jan 23, 2020

Uh oh!

aliok commented Jan 24, 2020 •

edited

Loading

Uh oh!

hasbro17 Jan 27, 2020

Uh oh!

estroz Jan 27, 2020

Uh oh!

camilamacedo86 Feb 4, 2020

Uh oh!

camilamacedo86 left a comment

Uh oh!

camilamacedo86 Feb 4, 2020

Uh oh!

hasbro17 commented Feb 4, 2020

Uh oh!

openshift-ci-robot commented Feb 4, 2020

Uh oh!

hasbro17 commented Feb 4, 2020

Uh oh!

aliok commented Feb 5, 2020

Uh oh!

hasbro17 commented Feb 7, 2020

Uh oh!

Uh oh!

		@@ -18,6 +19,8 @@

		### Removed

		- Breaking Change: Removed CSV configuration file support in favor of including files as input to the generator using [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options), defaulting to the `deploy/` directory. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))

internal/generator/olm-catalog: implement CSV generator #2407

internal/generator/olm-catalog: implement CSV generator #2407

Uh oh!

Conversation

estroz commented Jan 14, 2020

Uh oh!

estroz commented Jan 22, 2020

Uh oh!

aliok commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Need build/Dockerfile.

2. Execution of command won't copy the CRD file.

3. Package and CSV is put into the same directory.

4. Need GO111MODULE="on"

Uh oh!

estroz commented Jan 23, 2020

Uh oh!

aliok commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasbro17 Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

estroz Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

hasbro17 commented Feb 4, 2020

Uh oh!

openshift-ci-robot commented Feb 4, 2020

Uh oh!

hasbro17 commented Feb 4, 2020

Uh oh!

aliok commented Feb 5, 2020

Uh oh!

hasbro17 commented Feb 7, 2020

Uh oh!

Uh oh!

aliok commented Jan 23, 2020 •

edited

Loading

1. Need `build/Dockerfile`.

4. Need `GO111MODULE="on"`

aliok commented Jan 24, 2020 •

edited

Loading