:sparkles: Add new Golang base for go/v4-alpha with changes requested by the community in the layout

[camilamacedo86]

trafficstars

Description

• Add a new golang language base plugin (base.go/v4)
• Change the go/v4-alpha plugin to use the new base
• Address the layout changes requirements for go/v4. Now, the api and controllers are under the pkg dir.
• Change the Makefile to install kustomize with go install
• Remove crdVersion and webhook versions that is not supported and should no longer be served.

Motivation

Closes: https://github.com/kubernetes-sigs/kubebuilder/issues/932 Closes: https://github.com/kubernetes-sigs/kubebuilder/issues/2531

[camilamacedo86]

/hold

To allow more reviews.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[camilamacedo86]

/hold cancel

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[camilamacedo86]

/hold

We should not merge until we are able to discuss this.

[erikgb]

After sleeping on it, I wonder if using some kind of strategy pattern could make it possible to support multiple layouts. As it seems like users are preferring different layouts, or is this just a false assumption? That could also avoid some complex conditionals in the code - that I personally find hard to read, which are also duplicated to some extend. Also, the legacy/non-legacy layout abstraction is not extendable (add more variants) or sustainable (what to do when the new becomes legacy?). WDYT?

[camilamacedo86]

Hi @erikgb,

Thank you for your input. Following the answers inline.

I did an initial scan and added a few comments. More adoption of "Standard Go Project Layout" is something I would like to see, but IMO the controllers package should be internal, instead of pkg, as suggested by @shaneutt in https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-1255279059.

We need to have access to the controllers via the main.go . The main.go should be under cmd and cannot be placed under pkg. It is not possible to be done. However, if I missing something could you please for example change manually the layout of a sample https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4 and share with us how do you think that it should be done?

See: https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-1264403429

After sleeping on it, I wonder if using some kind of strategy pattern could make it possible to support multiple layouts. As it seems like users are preferring different layouts, or is this just a false assumption? That could also avoid some complex conditionals in the code - that I personally find hard to read, which are also duplicated to some extend. Also, the legacy/non-legacy layout abstraction is not extendable (add more variants) or sustainable (what to do when the new becomes legacy?). WDYT?

The if conditions (isLagacyLayout or not) should be removed when we are able to remove the go/v3 plugin and these implementations are only required for the tool to ensure backward compatibility. It will be removed and will only be kept until we are able to deprecate and remove the go/v3 plugin (such as done in the past)

It is not really easy for the tool to work with many layouts that bring many complexities. (See: https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-524984343 and https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-1264651286). However, users are able to create their own plugins with their own layouts. (see: https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-1268459400 )

We are here discussing here change the default layout only. However, if anyone would like to try to propose a solution for the tool/plugins works with multiple layouts then it is great. (So far no achievable solution was proposed). On top of that, it is out of our scope here and (ihmo) it would better fit in a design doc (https://github.com/kubernetes-sigs/kubebuilder/tree/master/designs) so we are able to discuss it properly.

[erikgb]

We need to have access to the controllers via the main.go . The main.go should be under cmd and cannot be placed under pkg. It is not possible to be done. However, if I missing something could you please for example change manually the layout of a sample https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4 and share with us how do you think that it should be done?

@camilamacedo86 Good idea! This should show what I suggest as the new default layout (minimal changes): https://github.com/kubernetes-sigs/kubebuilder/compare/master...erikgb:kubebuilder:new-project-structure

[camilamacedo86]

Hi @erikgb,

We need to have access to the controllers via the main.go . The main.go should be under cmd and cannot be placed under pkg. It is not possible to be done. However, if I missing something could you please for example change manually the layout of a sample https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4 and share with us how do you think that it should be done?

@camilamacedo86 Good idea! This should show what I suggest as the new default layout (minimal changes): https://github.com/kubernetes-sigs/kubebuilder/compare/master...erikgb:kubebuilder:new-project-structure

Did you try to build? That will not work out:

$ make build

cmd/main.go:22:2: use of internal package sigs.k8s.io/kubebuilder/testdata/project-v4/pkg/internal/controllers not allowed

[erikgb]

Did you try to build? That will not work out:

Now I am not sure what's going on. 😉 Am I doing something wrong? Of course I built it! Tests also work, BTW... I've also tried this layout on one of our own operators.

D:~/projects/github/kubebuilder/testdata/project-v4 (new-project-structure) $ make
test -s /home/erikbo/projects/github/kubebuilder/testdata/project-v4/bin/controller-gen && /home/erikbo/projects/github/kubebuilder/testdata/project-v4/bin/controller-gen --version | grep -q v0.10.0 || \
GOBIN=/home/erikbo/projects/github/kubebuilder/testdata/project-v4/bin go install sigs.k8s.io/controller-tools/cmd/[email protected]
/home/erikbo/projects/github/kubebuilder/testdata/project-v4/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/home/erikbo/projects/github/kubebuilder/testdata/project-v4/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
go build -o bin/manager cmd/main.go
D:~/projects/github/kubebuilder/testdata/project-v4 (new-project-structure) $

[camilamacedo86]

HI @erikgb,

Thank you for the help. Regards: https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1327665944:

Now I am not sure what's going on. 😉 Am I doing something wrong? Of course I built it! Tests also work, BTW... I've also tried this layout on one of our own operators.

I asked for you to provide an example because I thought that I was missing something. But see the following explanation as well. How can we be able to access the /pkg/internal/controllers from /cmd/main ? I might be wrong but it does not seem possible.

From: https://stackoverflow.com/questions/59342373/use-of-internal-package-not-allowed

Internal packages (packages that are inside a folder that has an internal folder in their path) can only be imported from packages rooted at the parent of the internal folder.

E.g. a package pkg/foo/internal/bar can be imported by the package pkg/foo/internal/baz and also from pkg/foo/baz, but cannot be imported by the package pkg nor can it be imported by pkg/bar. This is by design. This is so big, complex packages can be broken into smaller packages without having to expose internals.

You have to treat internal packages as "private" or non-existent from the "outside".

[erikgb]

Now I finally understand why there has been a lot of discussions in the PR! I think we need to agree on the goal here! I've created an overview of some alternative layouts, but feel free to add more:

├── alt-1
│   ├── api
│   ├── controllers
│   └── main.go
├── alt-2
│   ├── cmd
│   │   └── main.go
│   ├── internal
│   │   └── controllers
│   └── pkg
│       └── api
├── alt-3
│   ├── api
│   ├── cmd
│   │   └── main.go
│   └── internal
│       └── controllers
└── alt-4
    └── pkg
        ├── api
        ├── cmd
        │   └── main.go
        └── internal
            └── controllers

1 is the current layout, and 2 is closest to following the Go Standard project layout AFAIK. IMO 3 could also be considered to be within the standard, though the api directory should normally contain API specs in e.g. OpenAPI formats. But since the Go api types act as the master for the APIs, I think I could make sense. WDYT?

[camilamacedo86]

Hi @erikgb,

Thank you for all help and inputs Regards: https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1328026732

I can go along with alt-2 +1. 👍 (internal/controllers in the root dir)

[camilamacedo86]

/hold cancel

It seems good enough to get merged now. Thank you for all helps and reviews

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, umangachapagain

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[joelanford]

I'm personally not a huge fan of moving api/apis under pkg.

IMO the entire point of the APIs is that they are meant to be used by external users. As such, burying the APIs inside a pkg directory makes them less discoverable, and it potentially mixes them with other library code an operator author might decide needs to exist.

Personally, I'd rather see something like this:

├── apis
│   ├── v1
│   └── v1alpha1
├── cmd
│   └── controller
│       └── main.go
├── internal
│   └── controllers
└── pkg
    ├── client
    ├── image
    ├── log
    ├── util
    └── whatever

NOTE: two tweaks I'm suggesting are:

• leave apis at the root for discoverability
• put main.go in cmd/controller (this makes it easier to add other binaries alongside cmd/controller)

[erikgb]

I must say that I largely agree with the last comment from @joelanford (https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341033815). IMO we should also use the singular word (api) on the api directory, regardless of one/many APIs. Having so many "nobs" just adds a lot of complexity in the code and for users.

[camilamacedo86]

HI @shaneutt, @A-Hilaly, @jakobmoellersap, @moserke,

Could you please let us know wdyt about https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341033815? Is that make sense? Are you OK with we keep the apis in the root instead of create the pkg?

[mhrivnak]

Some operator/controller projects have found it necessary to make the api package a separate go module. That enables other projects (such as another operator) to import the structs without bringing in all of the dependencies of the controller, which can often cause a lot of problems if the two projects don't happen to align on their dependencies.

I suggest not moving the api package, so that this pattern can continue to be possible. I think it's even worth considering as the default, to make the controller's APIs more usable.

[shaneutt]

Keeping in mind that a big reason for making this change is to help manage how controllers with externally consumable library code are used from other projects I'm not sure I agree on the impetus for changing this. It seems to me that the api types are in fact just "part of the library" from the outside usage perspective, and having some of the library under pkg/ and some outside seems inconsistent, and maybe a bit against the spirit of what the PR originally set out to do (e.g. "follow Golang Standards").

That said, my primary concern is that we enable the ability to shield code from the outside that should not be consumable from other projects (e.g. controllers should be able to live in internal/ and that work with kubebuilder layout) so if others aren't persuaded by my take on this and the majority wants to go with the api in root approach that doesn't conflict with what I feel is the main concern.

[camilamacedo86]

Hi @mhrivnak,

https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341074263

Could you please clarifies your comment? What do you mean with I suggest not moving the api package, so that this pattern can continue to be possible?

It seems for me that you are worried about change the layout can broke projects. If so, the layout changes in for the next alpha plugin version so it will not break pre-existent projects. In the future, users will need at some point migrate their project to the go/v4-alpha and then follow the migration guide. So, if that is your concern (impacted in the existent projects) then. that does not seems a point valid in the scope of the changes.

[mhrivnak]

Could you please clarifies your comment? What do you mean with I suggest not moving the api package, so that this pattern can continue to be possible?

I think the api package should stay at the root of the repo, so that projects will continue to have the option to make it an independent go module.

[camilamacedo86]

Hi @joelanford, @erikgb,

I understand that the initial thought was keep the apis and controllers under root for discoverability as described in https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341033815. However, users prefer that their project follows the Golang Standards https://github.com/kubernetes-sigs/kubebuilder/issues/932 (20 votes). So following some questions:

• Therefore, have the apis under the pkg really makes harder for the users discovery them out?
• Also, would really make sense have the apis in the root and the pkg instead of all together?
• Why/when we would like to import the project (pkg) and not the APIs?

My 2 cents here is that if we do not add the apis under pkg in the practical terms what will happens is:

• a) People will change their scaffold to follow Golang common Standards
• b) Then will be harder for they update their project with the changes made and etc
• c) They will raise another issue that will be voted a lot to move the apis under the pkg/

So, I still more inclined to go along with https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341135881. But I would like to get your answers and further thoughts if possible as well.

[pmalek]

Dropping my 2 cents as well into the mix: I don't have a hard opinion on putting apis under pkg/ or not but I do see a benefit of doing so my making it "standard" as per the #932 and also thanks to separation of directories in a way that:

• we have cmd/ for app's entrypoints (main.go, flags and such)
• pkg/ for all the code that is exportable and usable from outside
• internal/ where folks can put their controllers code and anything else that they don't want to share outside (make part of the API contract)

Hence my slight 👍 on putting apis into pkg/.

[mhrivnak]

However, users prefer that their project follows the Golang Standards https://github.com/kubernetes-sigs/kubebuilder/issues/932 (20 votes).

It's good to be clear that there's not a standard, just conventions.

"It's not an official standard defined by the core Go dev team; however, it is a set of common historical and emerging project layout patterns in the Go ecosystem. Some of these patterns are more popular than others."

That pattern also describes many root-level directories, including an /api directory.

[shaneutt]

I think the api package should stay at the root of the repo, so that projects will continue to have the option to make it an independent go module.

I think it may be important to note that being under pkg/ does not preclude the notion of doing this.

a) People will change their scaffold to follow Golang common Standards b) Then will be harder for they update their project with the changes made and etc c) They will raise another issue that will be voted a lot to move the apis under the pkg/

If we make the kubebuilder standard different than the go standard, then I think these are true: we'll just continue to repeat the cycle by not having aligned standards.

[mhrivnak]

I think it may be important to note that being under pkg/ does not preclude the notion of doing this.

I might be behind on the times! Is it possible now to have a go module nested within another go module? I'm not sure how that would work. At a minimum it seems like it would be confusing, but I'm interested to know if and how people are doing this.

[pmalek]

I think it may be important to note that being under pkg/ does not preclude the notion of doing this.

I might be behind on the times! Is it possible now to have a go module nested within another go module? I'm not sure how that would work. At a minimum it seems like it would be confusing, but I'm interested to know if and how people are doing this.

You might want to look at a prominent example in the wild: https://github.com/open-telemetry/opentelemetry-collector-contrib with all the processors, exporters and receivers having their own respective modules e.g. https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/resourceprocessor (+ the naming of the tags for nested modules).

[RealHarshThakur]

I feel like having apis under pkg/ might get a bit weird if it ever got its own IDL(and I do hope it happens someday to collaborate easily with other language ecosystems). In the gRPC/proto world, I've seen usally the IDL in a different folder than the server stub and client.

[joelanford]

From the linked Go conventions doc about /pkg:

This is a common layout pattern, but it's not universally accepted and some in the Go community don't recommend it.

It's ok not to use it if your app project is really small and where an extra level of nesting doesn't add much value (unless you really want to :-)). Think about it when it's getting big enough and your root directory gets pretty busy (especially if you have a lot of non-Go app components).

So even the official conventions docs hesitate to really get behind it as a convention.

My take is that kubebuilder is scaffolding out fairly small projects. Would kubebuilder init or kubebuilder create api scaffold anything in /pkg other than /pkg/api? To me, this falls under what the doc says is "an extra level of nesting that doesn't add value", and IMO it slightly devalues the project because it hides the one thing that will be importable from other projects.

I could maybe see an argument for nesting APIs under pkg if kubebuilder also scaffolded other library code intended for external consumption.

And maybe it seems trivial, but the extra segment of the import path means consumers of the API, IDEs, etc. have to jump through one more directory (or poke around a little more on pkg.go.dev) before they find what they're looking for.

I'm not strongly against this (I see examples of both styles in upstream kubernetes), but my preference is for the APIs to live at the root.

Maybe as a compromise, we could make some of these directories relocatable. Default to /api, but add some flags to kubebuilder init that allow /api to be renamed to /pkg/apis or /joes/weird/path/to/myapis, and then store that choice in the PROJECT file so that future runs of kubebuilder create api would know where to do its thing?

[camilamacedo86]

Hi @joelanford,

Thank you a lot for all input.

Regards the comment https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341952427 I do not see how we would we could make it configurable and maintainable. However, I think that would be great if possible but would be better it be properly discussed via a design/PE doc. In this away, I think the goal for now is only define what is our default layout according what has been requested by the community.

[camilamacedo86]

I think we could achieve and convey to an agreement here:

Based on the comments

• https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341033815
• https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341952427
• https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341610812
• https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341177421
• https://github.com/kubernetes-sigs/kubebuilder/pull/2985#issuecomment-1341048965

And based on the answer of: If we add the apis/ under root would we still following the Golang Standard?

That pattern also describes many root-level directories, including an /api directory.

Obs.:IMO also makes more sense have the api in the root when I thinking about that import the apis from on operator project to another seems double as a good approach in the common cases scenarios either.

In this I will be working on this one to address the agreement. :tada: Thank you a lot for all contributions.