generate: add packagemanifests subcommand for new project layouts

[estroz]

Description of the change:

• operator-sdk generate packagemanifests with:
  • --kustomize:
    • creates or updates config/packagemanifests/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.
    • creates config/packagemanifests/kustomization.yaml if it does not already exist.
  • --manifests: reads either from stdin or on-disk files and applies them to a base to write CSV and CRD manifests to config/packagemanifests/<version>. This command emulates kustomize build.

Motivation for the change: See operator-framework/enhancements#16.

TODO's:

• rewrite tests in ginkgo/gomega
• add comments

[estroz]

/cc @hasbro17 @camilamacedo86 @jmrodri

[estroz]

Something to think about: perhaps a better name would be generate package.

[jberkhahn]

1. when i just run operator-sdk generate packagemanifests it prompts me for a bunch of stuff, and then blows up because --version is required?

jberkhahn@Purgatory> operator-sdk generate packagemanifests
.
/lines omitted/
.
Bases generated successfully in config/packages
Error: invalid command options:  is not a valid semantic version: Version string empty
Usage:
  operator-sdk generate packagemanifests [flags]

Flags:
      --apis-dir string        Root directory for API type defintions
      --channel string         Channel name for the generated package
      --crds-dir string        Root directory for CustomResoureDefinition manifests
      --default-channel        Use the channel passed to --channel as the package manifest file's default channel
  -h, --help                   help for packagemanifests
      --input-dir string       Directory to read existing package manifests from. This directory is the parent of individual versioned package directories, and different from --manifest-root
      --interactive            When set or no package base exists, an interactive command prompt will be presented to accept package ClusterServiceVersion metadata
      --kustomize              Generate kustomize bases
      --manifests              Generate package manifests
      --operator-name string   Name of the packaged operator
      --output-dir string      Directory in which to write package manifests
  -q, --quiet                  Run in quiet mode
      --stdout                 Write package to stdout
      --update-crds            Update CustomResoureDefinition manifests in this package
  -v, --version string         Semantic version of the packaged operator

Global Flags:
      --verbose   Enable verbose logging

FATA[0033] invalid command options:  is not a valid semantic version: Version string empty

that's a lot of spew. maybe add a default value (1.0.0?), and I think we should add the -version flag to the example usage. It also looks like errors are bubbling up from multiple places? I'm not sure whatFATA[0033] is from.

2. when i run it with --version set, i see "--manifest-root must be set if not reading from stdin" im not sure what this flag is from, i dont see it listed in the flags in -h. I guess this is because we're not sure if we want to use this name so you marked it as hidden? Not sure how we can release a command with a hidden required flag.

3. How do I set it to pull from stdin? I thought that would be --interactive, but it prompts me for a bunch of info even if I don't set --interactive, and when I do set --interactive nothing seems to be different? i guess this is because I'm running this on a project with no packages, but then I don't understand what it means to read from stdin.

4. the default behavior of --kustomize and --manifests being true unless you set one or the other is very strange and unintuitive. If we want to stick with this I would change the flag descriptions to something like --kustomize ONLY generate kustomize bases

5. Trying to write this fully formed is a very big bite. I think it might be beneficial to do a simple MVP and build from there for something like this.

more to come, I gotta go over the tests

[estroz]

@jberkhahn thanks for giving this a test run!

1. Validation should be run before any scaffold logic executes, and it currently isn't. Example usage is incoming, and will be similar to this. I'll make sure to add --version to it.
2. The name for --manifest-root needs to be decided on before making this CLI public for sure, which is why I initially hid it. I'm ok with the name, but iirc @hasbro17 said it sounds too much like manifests/ because it indicates a bundle format. Thoughts?
3. --interactive is on by default, but will only prompt you if a CSV doesn't already exist. To read from stdin, use a basic unix pipe like kustomize build config/bundle | operator-sdk generate packagemanifests. This isn't clear so I'll add a note in the flag help about piping.
4. That's interesting, there was some back and forth about this UX earlier and we came to a consensus on "all on by default". Can you read through that comment thread to see if the opposite argument makes sense?
5. generate packagemanifests is effectively a copy of generate csv for new project layouts, although organized differently. Is the --kustomize vs --manifests thing overwhelming, or something else?

[jberkhahn]

Is this still WIP? I was reviewing it as if it weren't because the label got taken off. just some quick responses to some of your points.

1. ok
2. I'm not 100% what this is supposed to point to. Is it the dir that contains the Kube yaml's for the operator? i.e. the old deploy dir?
3. oh, a pipe. Yeah, I thought interactive mode was what it meant by reading from stdin.
4. Both on by default is fine, what I think is weird is explicitly setting one turns the other off. I think the flags would make more sense if they were something like --only-kustomize or the flag descriptions mentioned that they make them exclusive.
5. It's difficult to attempt to reflexively test every option and combination of options for this much stuff. I'm just trying to make sure everything works and everything is properly tested.

[estroz]

2. --manifest-root should point to whatever contains your kube manifests, ex. Deployments and RBAC. The flag name is general because it could either point to deploy (in legacy project layouts) or some custom manifests directory, ex:

$ mkdir custom
$ kustomize build config/default > custom/operator-manifests.yaml
$ operator-sdk generate packagemanifests --manifest-root custom/

4. I'll update flag help.
5. Yeah totally understand. Given comments in operator-framework/enhancements#16 the CLI should be simplified soon (once that EP is merged).

[jberkhahn]

I think it might be worth it to separate some of this out into functions instead of declaring it all inline. Maybe a validate() and a run() like in some of the other commands?

[estroz]

This would be better in a follow-up because both packagemanifests and bundle are organized as such.

[jberkhahn]

It seems very little of the cmd layer of the CLI is tested. Do we have any goals to add tests for all of this? I'm nervous about this much untested code.

[estroz]

We have some command-level tests here for legacy project commands, and will likely add similar tests for new project commands. The "happy path" for bew project commands is tested in these e2e tests, and we have plans to add more.

[jberkhahn]

do you think it would be worthwhile to add unit tests here?

[estroz]

Most of these command files are just configuration for the underlying generator, so at this point unit tests would just be testing different inputs which is covered by the e2e and subcommand tests I linked above. I'd like to add subcommand tests in a follow-up. However I will add an e2e test here.

[jberkhahn]

I'd be down to hash out some unit tests for these bits.

[jberkhahn]

implementation seems good 👍

[jberkhahn]

one more thing, I think the generator overwrites a pre-existing file, right? Might want to add a test for that so that is explicitly intended behavior.

[estroz]

@jberkhahn file overwrites (CSV updates/upgrades) are tested here. We should make sure those tests cover more scenario's.

[hasbro17]

Still going through the rest. Works pretty much like the old csv generator which is good but the new workflow of reading manifests from stdin needs an example.

[hasbro17]

Also @estroz maybe this is just a limitation in the new workflow, but given that we don't have a config/packages/kustomization.yaml like we do for config/bundle/kustomization.yaml, the CSV generator doesn't get the example CR manifests from config/samples fed into stdin via the following:

$ kustomize build config/default | operator-sdk generate packagemanifests ...

And so the metadata.annotations.alm-examples field won't be populated like it does for generate bundle.

The old layout would be fine since --deploy-dir=deploy/ has the example CRs.

Any reason we can't have the same config/package/kustomization.yaml for generate packagemanifest?

resources:
- ../default
- ../samples

[hasbro17]

Also I don't want to rethink and redo the UX here.
Just want to double check we're still on track with the EP which has the following workflow:

$ kustomize build config/packages | operator-sdk generate packagemanifests -q --channel beta --version 0.0.1

Which led me to think there should be a config/packages/kustomization.yaml to build from.
Unless there actually is, and I messed up on generating it somewhere.

[estroz]

@hasbro17 the EP example should have included

$ cp config/bundle/kustomization.yaml config/packages

After some thought however the flow I'd like to see users follow is:

$ operator-sdk init
$ operator-sdk create api
$ operator-sdk generate packagemanifests --kustomize
$ kustomize build config/bundle | operator-sdk generate packagemanifests --version 0.0.1

since we want to push users towards using bundles by setting up config/bundle by default. This will be documented in both command help and website docs, the latter of which I was hoping to do as a follow-up but will add here to help explain UX.

We could take the above workflow a step further and set the default location for packagemanifests kustomize bases to be config/bundle so when a user wants to migrate to bundles they only have to run:

$ cp config/packages/0.0.1/ config/bundle/manifests
$ operator-sdk generate bundle --metadata

While the UX may seem a little more confusing at first, I don't think it actually is because all operator-framework manifest kustomize bases and the kustomization.yaml would be housed in config/bundle at all times, reducing confusion about having two different sets of kustomize-related files.

/cc @joelanford @camilamacedo86 @jmrodri

[joelanford]

@estroz I may not be totally following your above comment, but IMO, the packagemanifest CLI shouldn't depend on someone having already run generate bundle and having a config/bundle directory.

I think users would expect the following sequence of commands to work:

operator-sdk init ...
operator-sdk create api ...
operator-sdk generate packagemanifests -q --kustomize
kustomize build config/packages | operator-sdk generate packagemanifests --version=0.1.0

If we want to push users toward bundles, I think there may be other better ways of doing that (docs, log messages when using packagemanifests, etc.)

Also for sake of consistency and discoverability, should we rename config/packages to config/packagemanifests?

[estroz]

@joelanford to clarify: init will scaffold config/bundle/kustomization.yaml, which is the only requirement to run

$ kustomize build config/bundle | operator-sdk generate packagemanfiests

The only feasible way I see to scaffold a config/packages/kustomization.yaml in init is to use a flag option.

I'm fine with config/packagemanifests for those reasons.

[joelanford]

This is mostly a nit, since I don't think it matters that much.

What would you think about having the CSV generated in config/packagemanifests/bases contain ONLY the metadata that we can't idempotently recompute, and then when we're reading Go files and adding things based on what APIs exist and what markers exist in those APIs, we create a CSV patch file per CRD.

That way:

• we never overwrite the base CSV.
• we always overwrite the CSV patches.

This approach may mean having to touch the kustomization.yaml to add patch files as we add APIs (not sure if kustomize has a way to say "apply all patches in this directory").

Would this get us out of doing merging into the base CSV?

[estroz]

@joelanford ultimately that's what we should implemented have before even moving to full kustomize building the CSV. The only thing I worry about is what olm.yaml will look like in the future, and whether that will contain spec.customresourcedefinitions and other UI fields generated from API definitions. Then we'll effectively be going back to regenerating the base. @ecordell is there a definition of olm.yaml in an EP somewhere?

Also controller-gen will regenerate bases, not patches, which is effectively what generate <bundle|packagemanifests> is doing now. Generating patches would depart from doing that, which is ok with me but still something to be aware of.

[joelanford]

Also controller-gen will regenerate bases, not patches, which is effectively what generate <bundle|packagemanifests> is doing now. Generating patches would depart from doing that, which is ok with me but still something to be aware of.

True, but isn't controller-gen fully idempotent, where it just rewrites the whole base without considering what's already there?

[hasbro17]

So --update-crds=false as the default implies the opposite of the help text This will occur automatically when a new package is generated.

Is it possible to simplify this by just having --update-crds=true as the default.

And that simplifies our internal logic at L222 as just:

if c.updateCRDs {
		var objs []interface{}
...

without having to do L181-L183:

// When generating a new package, CRDs should be written unless --update-crds has been explicitly set to false.
updateCRDsOff := c.updateCRDsFlag.Changed && !c.updateCRDs
writeNewPackageCRDs := !updateCRDsOff && genutil.IsNotExist(packageDir)

[estroz]

The UX gets a little weird here because you can update older package manifests versions of your operator, but you may not necessarily want to update CRDs for that version. That was the original thought at least.

Now that I think about this, I can imagine that developers tend to work on the latest package manifests version in master then check out the related release branch if they need to work on a prior version, in which the older version is latest. Given that premise I am ok with defaulting to --update-crds=true. Thoughts @joelanford?

[hasbro17]

A few non-blocking nits but LGTM.

[hasbro17]

Align spacing with the rest of the example.

Suggested change

	# Generate manifests then create the package manifests base:
	# Generate manifests then create the package manifests base:

[hasbro17]

Super pedantic nit but Expect(err).Should(Succeed()) just sounds a little odd when I read it out loud.

From the godocs for Suceed() it's meant to be passed the function and not the error it seems. And a single error return function at that.

Succeed passes if actual is a nil error Succeed is intended to be used with functions that return a single error value. Instead of

    err := SomethingThatMightFail()

    Expect(err).ShouldNot(HaveOccurred())

You can write:

    Expect(SomethingThatMightFail()).Should(Succeed())

If we're getting the err value first then Expect(err).ShouldNot(HaveOccurred()) reads better.

But non-blocking.

[estroz]

I agree, these should be Expect(err).NotTo(HaveOccurred()), which is idiomatic. Will fix this in a follow-up.