commands/operator-sdk: rename to cmd/operator-sdk and reorganize

[joelanford]

Description of the change:

• Moving commands/operator-sdk/cmd to cmd/operator-sdk/
• Collapsing commands/operator-sdk/cmd/<subcommand> to commands/operator-sdk/<subcommand>

Motivation for the change:
Necessary for 1.0.0 release and closes #1184

[AlexNPavel]

LGTM, just one question about one of the lower packages.

[lilic]

lgtm apart from the one suggestion

[shawn-hurley]

This feels wrong to me.

I don't think that anyone expects the cmd/ package to be importable, and no other CLI/Library does this that I can tell. I don't think this will break anything, but it just seems unnecessary and will confuse folks who come to contribute IMO.

[joelanford]

@shawn-hurley the Go project layout convention states:

Don't put a lot of code in the application directory. If you think the code can be imported and used in other projects, then it should live in the /pkg directory. If the code is not reusable or if you don't want others to reuse it, put that code in the /internal directory. You'll be surprised what others will do, so be explicit about your intentions!

I agree with that statement, especially that last sentence. To better align with this, I'll move commands/operator-sdk/cmd under the root /internal directory, as described, and we'll see how that looks.

[shawn-hurley]

I just don't see why many packages don't need to do this and we decided we do.

https://github.com/kubernetes-sigs/kubebuilder
both pkg/ and cmd/

https://github.com/kubernetes/kubernetes
both pkg/ and cmd/

https://github.com/kubernetes-incubator/service-catalog
both pkg/ and cmd/

and I am sure we can find more. We should look like the rest of the ecosystem.

[joelanford]

@shawn-hurley Maybe we don't need to do this, but it seems like a harmless change that has the added benefit of explicitly removing packages from our public API that we don't intend for others to use. I think by leaving things as they are, we are making the statement that they're importable (humans may or may not know our intentions, but tools like goimports definitely don't).

There are lots of imports of k8s.io/kubernetes/cmd in other repos, so in that case it seems the Kubernetes maintainers have either decided to make those packages part of their public API or they have been ineffective in getting users not to import them.

In any case, I'm okay closing this PR if the consensus is that we're okay leaving the existing layout in place.

[hasbro17]

We technically only have 1 command/executable(pkg with main identifier) that's commands/operator-sdk/main.go.
All other pkgs that we have at commands/operator-sdk/cmd/* are just utils/helpers for that main cmd. And those are not meant to be imported outside the project.

Ideally we should rename commands/operator-sdk/main.go=>cmd/operator-sdk/main.go.
And the non-main helper pkgs then become cmd/operator-sdk/add, cmd/operator-sdk/generate etc. That format is more in line with the unofficial guidelines for cmd pkgs.

And since those helper pkgs are meant to only be used by cmd/operator-sdk ideally they'd become cmd/operator-sdk/internal/generate.
But in practice I can see shawn's point on the rest of the ecosystem not doing this.
https://github.com/kubernetes/kubernetes/tree/master/cmd/cloud-controller-manager/app
Beyond the kube ecosystem I can even see some examples in upstream Go code where you have exported identifiers in cmd pkgs.
https://github.com/golang/tools/blob/master/cmd/benchcmp/compare.go#L21

So I'm okay with relaxing the requirement of having cmd/operator-sdk/internal/* and instead relying on the expectation that those pkgs aren't meant to be imported.

Although I'd want to find out if the convention in the kube ecosystem is just an oversight or if those identifiers are meant to be for public use.

[lilic]

My two cents:

I agree about following the guidlines here, it seems to be well accepted in the community. It does also mention the use of internal directory in cmd (which I agree, we should rename command->cmd, as we do in the end generate an operator with cmd dir name, so we should be consistent besides following conventions)

...If the code is not reusable or if you don't want others to reuse it, put that code in the /internal directory. You'll be surprised what others will do, so be explicit about your intentions!

Also the discussion on this issue is interesting, I think Jaana (rakyll) made a good point and I feel like it's how majority of users see/use packages as well (regardless where the package might live):

Any importable package is for public use. Anything else goes into internal. Note that internal package is not just a convention, it is supported by the Go tool.

To sum up, if we are already making changes to the API and making things private/internal towards the 1.0.0 release we might as well cleanup a bit and follow the conventions. Personally I agree with the above and we should do the following in this PR instead:

commands/operator-sdk/cmd/add.go → cmd/operator-sdk/add/...

and any internal components we can move to:

cmd/operator-sdk/internal/...

Changed my mind :)

[joelanford]

Ok I've updated the PR to account for the recent comments. The commands directory has been renamed to cmd, and the subcommand packages have been collapsed down to cmd/operator-sdk/<subcommand>.

I moved the New*Cmd functions into the respective subcommand packages and (where possible) made the rest of the functions and idenifiers private. There were two exceptions:

1. In cmd/operator-sdk/generate, there are two functions (K8sCodegen() and OpenAPIGen()) that are called by the add subcommand, so they needed to remain public. Thoughts on leaving that dependency in place or extracting those to a shared internal library?
2. In cmd/operator-sdk/scorecard, there's a large number of exported identifiers. I imagine we'll want to move the public-facing helpers and APIs to pkg at some point, and unexport the rest. But since @AlexNPavel is making lots of changes here, I figured light touch was best.

@shawn-hurley @hasbro17 @lilic @AlexNPavel PTAL.

[lilic]

Looks good! 🎉

In cmd/operator-sdk/generate, there are two functions (K8sCodegen() and OpenAPIGen()) that are called by the add subcommand, so they needed to remain public. Thoughts on leaving that dependency in place or extracting those to a shared internal library?

Yes I would extract them to a shared internal lib, think cmd/operator-sdk/internal should work and it can be used by both the generate and the add packages. It shouldn't be part of our API I think. WDYT?

[joelanford]

@AlexNPavel @lilic @shawn-hurley @hasbro17 PTAL

I extracted the generator helpers into /cmd/operator-sdk/internal/genutil.

As a result, I had to refactor the way that the header file flag was handled in the OpenAPI generator. Now that the generator is in a separate package from its callers, it didn't make sense for the caller and callee to share a global variable for the header file.

Instead, I made an options struct that includes the header file and updated the genutil.OpenAPIGen() function to take that as a parameter. I figured that interface made sense since we're wrapping the openapi-gen CLI and may want to add/remove/change what flags we pass through from our CLI. Thoughts?

[estroz]

This build error is unrelated to these changes. Seems like an upstream breaking change, since my go mod PR which was previously passing is now failing for the same reason.

[joelanford]

This build error is unrelated to these changes. Seems like an upstream breaking change, since my go mod PR which was previously passing is now failing for the same reason.

Here's the error messages: https://travis-ci.org/operator-framework/operator-sdk/jobs/511514286#L910-L913

I'm fairly sure it's related to the changes we made to support all the client auth plugins.

I'll take a look at what's going on.

[hasbro17]

LGTM

[estroz]

LGTM after rebase.