Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package Root #932

Closed
moserke opened this issue Aug 12, 2019 · 35 comments · Fixed by #2985
Closed

Package Root #932

moserke opened this issue Aug 12, 2019 · 35 comments · Fixed by #2985
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@moserke
Copy link

moserke commented Aug 12, 2019

Add the ability to set the Go package root. Currently api and controller is created in the root of the project. It would be nice to be able to specify putting these packages in pkg to maintain good Go hygiene.

/kind feature

@moserke moserke added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 12, 2019
@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Aug 12, 2019

I suspect this'll have to wait until after we clean up the scaffolding code -- it'd add to the mess to implement this right now.

That being said, while I'm not against supporting this, I'd like to note that "pkg" isn't necessarily the "one true way" to do Go projects. Moving away from "pkg" was somewhat intentional -- while it was clear to Kubernetes contributors, the "pkg" and "cmd" bifrucation wasn't always clear to external users, who looked for a main.go file.

@moserke
Copy link
Author

moserke commented Aug 13, 2019

Agreed that it's not the only way, but personally I feel like the pkg & cmd separation is becoming the idiomatic way of laying out a project. Coming from more than just k8s repos it did feel a little out of the normal not to have that structure.

That said though, I completely understand how serving everyone is difficult and there is never going to be the perfect solution. Appreciate the willingness to look into supporting this.

Once the code is ready for more changes like this, I'm also happy to help implement this! Would enjoy getting more plugged in to the community.

@DirectXMan12
Copy link
Contributor

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Aug 13, 2019
@vincepri
Copy link
Member

/cc

@hudymi
Copy link
Contributor

hudymi commented Aug 26, 2019

Can two locations be considered? Root for api and separate root for controllers and webhooks. In our projects, we like to have core implementation that shouldn't be imported by any other project in the internal package and things like api in pkg. So for us, it would be convenient to have the ability to configure it.

@DirectXMan12
Copy link
Contributor

at a certain point, too many configuration settings makes this a bit weird. KB itself is supposed to be a recommendation for how to use things -- it's one of the reasons we try hard to keep generic stuff in controller-tools and controller-runtime, so that more complex and/or project-specific layouts can be used. We'll have to weigh the costs of "how do we prevent this from becoming a free-for-all" with each of the options.

@camilamacedo86
Copy link
Member

HI @DirectXMan12,

Could/Should we do it now as well or add to v3 project?

@DirectXMan12
Copy link
Contributor

prob v3 (or in a plugin) if we decide to do it at all.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 5, 2020
@Adirio
Copy link
Contributor

Adirio commented Mar 5, 2020

/lifecycle frozen
/milestone scaffolding-v3

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 5, 2020
@Adirio
Copy link
Contributor

Adirio commented Mar 5, 2020

@DirectXMan12 @mengqiy @droot can any of you add this to "scaffolding-v3" milestone, it seems I can't do it

@DirectXMan12 DirectXMan12 added this to the scaffolding-v3 milestone Mar 13, 2020
@DirectXMan12
Copy link
Contributor

done

@DirectXMan12
Copy link
Contributor

you need to be a maintainer to use /milestone at the moment, which is a weird designation that we carried over from Kubernetes but didn't actually want to use. Someone needs to fix the permissions so that reviewers & approvers can /milestone too, or something. The current permission set is weird.

@DirectXMan12
Copy link
Contributor

This seems like a decent candidate to use as a test for plugins phase 2 -- it's mostly a matter of preference, and should be something that's easily doable under our general deisgn currently.

@camilamacedo86
Copy link
Member

camilamacedo86 commented May 14, 2020

Hi @DirectXMan12,

I have been thinking about it. Personally, I do not believe that it could be addressed in the plugins phase 2 because who is asking it are the end-users who are looking for to use the Kubebuilder tool to scaffold the projects in this layout. Then, the plugins are useful for who is looking for this project as a lib to create their tools extending it such as SDK.

Also, I think that allows changing the path to scaffold the files will increase the complexity of the plugin phase 2 and would make hard address stories such as I'd like to build my project with kubebuilder but use the SDK features to integrate it with OLM.

Also, regards the argumentation over it be a matter of preference than shows that many folks have from the community has this preference since this change would make the scaffolded project follow up common practices. See for example; https://github.com/golang-standards/project-layout#pkg

@DirectXMan12
Copy link
Contributor

Personally, I do not believe that it could be addressed in the plugins phase 2 because who is asking it are the end-users who are looking for to use the Kubebuilder tool to scaffold the projects in this layout

Plugins phase 2 will most likely involve out-of-tree plugins. That means that we could have an example plugin for "scaffold under pkg".

Also, I think that allows changing the path to scaffold the files will increase the complexity of the plugin phase 2 and would make hard address stories such as I'd like to build my project with kubebuilder but use the SDK features to integrate it with OLM.

I'm not following, can you clarify here? Under what we've brainstormed for phase 2, this should be fairly trivial -- receive the world object, rewrite all .go path keys to have the extra pkg (except /main.go, which goes, under cmd), and then fix up the imports in those go files (parse with the go/xyz packages, edit the imports, then re-serialize with the same packages).

@camilamacedo86
Copy link
Member

Hi @DirectXMan12,

So, your idea is that the user could choose, I mean have an option in the KB commands to choose the plugin option to do scaffold as A or B or C. Am I right? Then, I agree that if other tools extending it also would still able to work with the N available options.

@DirectXMan12
Copy link
Contributor

@camilamacedo86 while I'm not exactly certain what the final shape of plugins part 2 would be, I'd expect you'd end up having the "move everything to pkg and cmd" plugin be the last one, so other things scaffold as normal and then that plugin move things around. That's how people would get to chose, and other scaffolding and plugins would still be able to work with both options.

@camilamacedo86
Copy link
Member

We have been discussing this issue. And we are thinking about accepting this one and changing the default layout into go/v4-alpha. Also, a suggestion made was to scaffold the controllers under the pkg/internal path.

Therefore, we will check it out better and we will raise this again in the next meeting to see if we can agree to a final decision. Please, feel free to attend if you would like to speak about it. More info

@shaneutt
Copy link
Member

Glad to hear it @camilamacedo86. I'm definitely in favor of the change, particularly to put the controllers under internal/ and the APIs under pkg. I wasn't able to make it to today's meeting, but I will come to the next one and see how best I can contribute. 👍

camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 1, 2022

Regards adding the controllers under internal:

  • It is not possible because we need to use the controllers in the main.go
  • Also, it might let the impression that would be recommended people import projects in another project when that does not seems like the best approach for the common case scenarios

c/c @shaneutt

camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 1, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2531
Closes: kubernetes-sigs#2822
@jgillich
Copy link

jgillich commented Oct 1, 2022

It is not possible because we need to use the controllers in the main.go

You can. Internal packages are accessible by packages sharing the path of its parent. Anything under foo.com/bar will be able to access foo.com/bar/internal.

Some people may still prefer to have controllers under pkg though. IMHO the best course of action is to make the paths configurable via the Makefile or PROJECT

camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 2, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2822
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this issue Oct 2, 2022
…munity in the layout

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

Closes: kubernetes-sigs#932
Closes: kubernetes-sigs#2822
@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 2, 2022

Hi @jgillich,

Some people may still prefer to have controllers under pkg though. IMHO the best course of action is to make the paths configurable via the Makefile or PROJECT

That is not achievable via Makefile or PROJECT one and it is out of scope.
If you would like to propose available and maintainable solutions to achieve the goal of we are able to have the paths configurable and working for all plugins please feel free to push a design doc so that we can discuss it. WDYT?

Note that users might want to create their own plugins and tools as well. So that they can define other's layouts and use them with kubebuilder CLI (using plugins phase 2 which is implemented already, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/extensible-cli-and-scaffolding-plugins-phase-2.md)

@shaneutt
Copy link
Member

shaneutt commented Oct 4, 2022

One thing I've never been clear on, is why is this type of option so hard to add to PROJECT so that individual projects can simply choose where they want to put packages?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 5, 2022

Hi @shaneutt,

One thing I've never been clear on, is why is this type of option so hard to add to PROJECT so that individual projects can simply choose where they want to put packages?

The project is a scaffold. Then, editing the PROJECT file will NOT change what was done already.
Then, I cannot see how that could work. Therefore, we have many plugins. dependences and options and ALL/ANY would need to have a way to change what was done already based on the changes in this file.

However, if you see options please, feel free to push a design doc and/or PR (better yet) with any implementation that you have in mind so we are able to have a better idea and discuss the approach.

@shaneutt
Copy link
Member

shaneutt commented Oct 5, 2022

To me it just seems like you would set in the project where your directory is, and while that action alone of course wouldn't change what was done already, it should allow the tooling to look there instead of the default place.

e.g. the following workflow seems like something we should be able to make work:

  1. move the (default) controllers/ directory to internal/controllers/
  2. update the PROJECT config and provide a custom value for where the controller packages live at internal/controllers/
  3. all tooling should thereafter work with the new location

@camilamacedo86 I (or one of my colleagues) may be able to put in a PR/KEP for this, but could you provide some assistance by pointing out some of the places in code where the directory structure is determined/handled? This could help provide something of a map to get started on this voyage and I would appreciate it if you could. 🙇

@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 5, 2022

HI @shaneutt,

To me it just seems like you would set in the project where your directory is, and while that action alone of course wouldn't change what was done already, it should allow the tooling to look there instead of the default place.

e.g. the following workflow seems like something we should be able to make work:

move the (default) controllers/ directory to internal/controllers/
update the PROJECT config and provide a custom value for where the controller packages live at internal/controllers/
all tooling should thereafter work with the new location

The files are scaffolded in the places where all plugins and the tool will be able to work with and support the operations such as markers and etc. See, for example, the comment: #932 (comment)

Add new specs to the PROJECT file, for them to allow/request users to do manual changes, as implement all CLI options, configurations like controller-gen as any plugin to be able to do the next actions accordingly is not maintainable and sustainable. We have no way (IMHO) to support that. We should only support what is possible to get done 100% by the tool. My vote is -1 for this approach.

Also, this issue is for us to discuss the default scaffold layout. I am OK with us adding the pkg as requested by the community and as conveyed before in this one. Then, to allow people to do their own customized scaffolds and helpers we added an API where kubebuilder can be used as LIB and allow anyone to create their own plugins that do the scaffolds as please them or that add things on top such as it is done by Operator-SDK.

Note that with plugins phase 2 (https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/extensible-cli-and-scaffolding-plugins-phase-2.md) which is implemented already, anyone can create their own plugin that does the scaffolds as desired. Therefore, users can re-use all Kubebuilder implementations to make them simple and easily maintainable and then, use their external plugin(s) with Kubebuilder. So, we can support the API and the default options.

@camilamacedo86
Copy link
Member

A PR was done with the request to add the api/controllers under the pkg directory: #2985

Please, feel free to check it out. The next step is to check the pros/cons and decide if we will not move forward with attending to the request made by the community.

c/c @jmrodri @varshaprasad96 @everettraven

@camilamacedo86
Copy link
Member

Based in the community feedback and latest discussions the convey for the layout changes to the new go/v4 plugin (see: #2985 (comment)) is:

  • The directory apis was renamed to api to follow the standard
  • The controller(s) directory has been moved under a new directory called internal and renamed to singular as well controller
  • The main.go previously scaffolded in the root directory has been moved under a new directory called cmd
    Therefore, you can check the changes in the layout results into:

...
├── cmd
│ └── main.go
├── internal
│ └── controller
└── api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet