generate: add `bundle` subcommand for new project layouts by estroz · Pull Request #3065 · operator-framework/operator-sdk

estroz · 2020-05-18T21:06:51Z

Description of the change:

operator-sdk generate bundle with:
- --kustomize: creates config/bundle/kustomization.yaml, and creates or updates config/bundle/bases with UI metadata. The base contains no manifests. This command will read API type data to generate customresourcedefinitions metadata using GVK's in PROJECT.
- --manifests: reads either from stdin or on-disk files and applies them to a base to write CSV and CRD manifests to config/bases/manifests. This command emulates kustomize build.
- --metadata: calls the same generator as the current bundle create --generate-only.

Motivation for the change: See #2860.

~~Depends on #3064.~~

estroz · 2020-05-19T00:36:52Z

/cc @hasbro17

hasbro17

Still going through but had a question on the --manifest-root flag.

hasbro17 · 2020-05-21T02:38:04Z

+		"Only set if creating a new bundle or upgrading your operator")
+	fs.StringVar(&c.inputDir, "input-dir", "", "Directory to read an existing bundle format from")
+	fs.StringVar(&c.outputDir, "output-dir", "", "Directory to write the bundle format to")
+	fs.StringVar(&c.manifestRoot, "manifest-root", "", "Root directory for operator manifests, ex. Deployment and RBAC")


@estroz So this is effectively --deploy-dir right?
I guess this is subjective but manifests immediately made me think of the bundle manifests directory.
I wonder if a better name wouldn't be operator-manifests-dir similar to crds-dir even if it's more verbose.
Also correct me if I'm wrong but this dir is used for Custom Resource manifests collection, and not the crd dir as mentioned below:

Suggested change

fs.StringVar(&c.manifestRoot, "manifest-root", "", "Root directory for operator manifests, ex. Deployment and RBAC")

fs.StringVar(&c.manifestRoot, "manifest-root", "", "Root directory for operator manifests: Deployment, RBAC and CustomResources.")

operator-manifests-dir doesn't seem different enough to me. Perhaps a better solution is to be explicit about the difference between --input-dir and --manifest-root in flag help.

hasbro17

LGTM

The Gomega tests are a joy to read. A far cry from our current csv unit tests. Great work on making it so readable.

Just one nit on making the internal errors constants.

Also I still think having the flags --manifests for bundle generation and manifest-root for operator manifests makes it seems like they're related when they're not.
But not going to hold up this PR over that since the flags have enough descriptions to highlight the difference.

hasbro17 · 2020-05-21T22:20:21Z

+
+			BeforeEach(func() {
+				tmp, err = ioutil.TempDir(".", "")
+				ExpectWithOffset(1, err).ToNot(HaveOccurred())


Why are we using ExpectWithOffset()?
Does Expect(err).ToNot(HaveOccurred()) not work here?

Unlike `Expect` and `Ω`, `ExpectWithOffset` takes an additional integer argument that is used to modify the call-stack offset when computing line numbers. This is most useful in helper functions that make assertions. If you want Gomega's error message to refer to the calling line in the test (as opposed to the line in the helper function) set the first argument of `ExpectWithOffset` appropriately.

Seems like this is to bubble up error to the line number of the caller and not the helper itself.

Exactly, and I've named all test helper functions that use gomega Expect*() functions with the Helper suffix.

camilamacedo86 · 2020-05-21T23:45:14Z

+		},
+	}
+
+	cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "Generate kustomize bases")


Suggested change

cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "Generate kustomize bases")

cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "If set, generate ONLY the kustomize config bases")

If you set --kustomize --manifests, both bases and manifests will be generated. I think giving this a simple help description is ok for now.

My suggestion is made clear that if we set the option than means that only that will be exec which in POV not so intuitive as described here: https://github.com/operator-framework/operator-sdk/pull/2860/files#r427888547

If you only set --kustomize, only kustomize bases are generated, as you'd expect. If you set another flag with --kustomize like --manifests, then both kustomize bases and manifests will be generated. I think that's what you're trying to say right?

camilamacedo86 · 2020-05-21T23:45:53Z

+	}
+
+	cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "Generate kustomize bases")
+	cmd.Flags().BoolVar(&c.manifests, "manifests", false, "Generate bundle manifests")


Suggested change

cmd.Flags().BoolVar(&c.manifests, "manifests", false, "Generate bundle manifests")

cmd.Flags().BoolVar(&c.manifests, "manifests", false, "If set, generate ONLY the bundle manifests")

camilamacedo86 · 2020-05-21T23:46:11Z

+
+	cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "Generate kustomize bases")
+	cmd.Flags().BoolVar(&c.manifests, "manifests", false, "Generate bundle manifests")
+	cmd.Flags().BoolVar(&c.metadata, "metadata", false, "Generate bundle metadata and Dockerfile")


Suggested change

cmd.Flags().BoolVar(&c.metadata, "metadata", false, "Generate bundle metadata and Dockerfile")

cmd.Flags().BoolVar(&c.metadata, "metadata", false, "If set, generate ONLY the bundle metadata and its Dockerfile")

camilamacedo86 · 2020-05-22T00:08:10Z

+	if err := fs.MarkHidden("manifest-root"); err != nil {
+		panic(err)
+	}
+


Can we just improve the info to clarifies by giving examples over what from where is expected usually? Eg. Directory to read an existing bundle format from. Eg

For --manifest-root? The help lists deploy and config as example values to pass. I expect we can improve this flag in #2860.

camilamacedo86

IMO it is just missing doc methods in order to allow others to understand where/when/why better as improving the flags info to make easier know what is expected in each case and their behaviour.

Otherwise, it is LGTM.

for a "new" project layout. cmd/operator-sdk/generate: bundle command; CustomResourceDefinition update behvior lives here internal/generate/clusterserviceversion: modular ClusterServiceVersion generator

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 May 18, 2020

openshift-ci-robot requested review from shawn-hurley and varshaprasad96 May 18, 2020 21:06

estroz force-pushed the feature/generate-bundle branch from c94c126 to de0633e Compare May 19, 2020 00:21

estroz changed the title ~~[WIP] generate bundle~~ generate: add bundle subcommand for new project layouts May 19, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2020

estroz force-pushed the feature/generate-bundle branch from de0633e to e801396 Compare May 19, 2020 00:36

openshift-ci-robot requested a review from hasbro17 May 19, 2020 00:36

estroz requested review from camilamacedo86 and joelanford May 19, 2020 00:37

estroz mentioned this pull request May 19, 2020

plugin: write Makefile recipes for generate bundle subcommand #2860

Merged

2 tasks

hasbro17 reviewed May 21, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/cmd.go Outdated

estroz force-pushed the feature/generate-bundle branch from 288b773 to 0da8689 Compare May 21, 2020 19:06

hasbro17 approved these changes May 21, 2020

View reviewed changes

estroz force-pushed the feature/generate-bundle branch from 0da8689 to 24ab500 Compare May 21, 2020 23:39

camilamacedo86 reviewed May 21, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/bundle.go Outdated

camilamacedo86 reviewed May 21, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/bundle.go

estroz force-pushed the feature/generate-bundle branch 2 times, most recently from c08c8c0 to 10527e2 Compare May 21, 2020 23:58

camilamacedo86 reviewed May 22, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/bundle.go Outdated

camilamacedo86 reviewed May 22, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/bundle.go Outdated

camilamacedo86 reviewed May 22, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/bundle/bundle.go

camilamacedo86 reviewed May 22, 2020

View reviewed changes

Comment thread cmd/operator-sdk/generate/cmd.go

camilamacedo86 approved these changes May 22, 2020

View reviewed changes

Eric Stroczynski added 2 commits May 21, 2020 17:15

This commit adds 'generate bundle' which will generate a bundle format

0972332

for a "new" project layout. cmd/operator-sdk/generate: bundle command; CustomResourceDefinition update behvior lives here internal/generate/clusterserviceversion: modular ClusterServiceVersion generator

generate bundle: hide --manifest-root until name is decided

ace0487

estroz force-pushed the feature/generate-bundle branch from 10527e2 to ace0487 Compare May 22, 2020 00:15

estroz merged commit 357491d into operator-framework:master May 22, 2020

estroz deleted the feature/generate-bundle branch May 22, 2020 01:49

estroz mentioned this pull request May 22, 2020

generate bundle: enable interactive mode, set metadata default #3087

Merged

	fs.StringVar(&c.manifestRoot, "manifest-root", "", "Root directory for operator manifests, ex. Deployment and RBAC")
	fs.StringVar(&c.manifestRoot, "manifest-root", "", "Root directory for operator manifests: Deployment, RBAC and CustomResources.")

	cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "Generate kustomize bases")
	cmd.Flags().BoolVar(&c.kustomize, "kustomize", false, "If set, generate ONLY the kustomize config bases")

	cmd.Flags().BoolVar(&c.manifests, "manifests", false, "Generate bundle manifests")
	cmd.Flags().BoolVar(&c.manifests, "manifests", false, "If set, generate ONLY the bundle manifests")

	cmd.Flags().BoolVar(&c.metadata, "metadata", false, "Generate bundle metadata and Dockerfile")
	cmd.Flags().BoolVar(&c.metadata, "metadata", false, "If set, generate ONLY the bundle metadata and its Dockerfile")

Conversation

estroz commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz commented May 19, 2020

Uh oh!

hasbro17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hasbro17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

estroz commented May 18, 2020 •

edited

Loading

camilamacedo86 May 22, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading