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

simplify package/dir structure in kpt CLI codebase #3297

Open
droot opened this issue Jun 9, 2022 · 4 comments
Open

simplify package/dir structure in kpt CLI codebase #3297

droot opened this issue Jun 9, 2022 · 4 comments
Labels
area/devops enhancement New feature or request triaged Issue has been triaged by adding an `area/` label

Comments

@droot
Copy link
Contributor

droot commented Jun 9, 2022

In alpha version of kpt, package structure was dictated with the desire to share command implementations between kpt and kustomize and that turned out to be bad idea because cmd implementations are closer to the UX of the tools and differ considerably.

I think now we have very clear idea what needs to be shared and what not, we can refactor to simplify the structure.

We can see contributors are having difficult time picking up package names for new commands. cmdrpkgfindupdates :)

https://github.com/GoogleContainerTools/kpt/pull/3290/files

/cc @mortent @natasha41575

@droot droot added enhancement New feature or request triaged Issue has been triaged by adding an `area/` label area/devops labels Jun 9, 2022
@mortent
Copy link
Contributor

mortent commented Jun 9, 2022

Given that we are moving towards providing the kpt functionality as a library, we should consider splitting it up into multiple modules. I'm thinking we could do:

  • github.com/GoogleContainerTools/kpt/cli: This could include the cobra commands and any functionality needed for the cli.
  • github.com/GoogleContainerTools/kpt/core: This is the core functionality of kpt, which will be used by both porch and the kpt cli.
  • github.com/GoogleContainerTools/kpt/porch: The porch aggregated apiserver. Porch itself also currently includes several go modules, but I haven't looked into detail at them yet.

This should create a better separation between the different components and avoid nested go modules (which works, but tend to be a bit confusing). The separation between the library and the cli is similar to Kustomize.

Using multiple go modules used to be somewhat hard to work with since it required replace directives, but it should be much easier with go workspaces in 1.18.

@droot
Copy link
Contributor Author

droot commented Jun 9, 2022

Sounds good to me. Excited to move it to Go 1.18.

@natasha41575
Copy link
Contributor

natasha41575 commented Feb 24, 2023

I agree with @mortent's comment above. And I assume we will have an additional github.com/GoogleContainerTools/kpt/rollouts now.

In kustomize, we use go workspace mode so that we did not have to remove and re-add the replace statements every time, but we ran into some issues with that and had to go back to having the replace statements. That said, the release process there is still not that bad and I think it is worth it.

Do we have a timeline on when we would like to do this, and is there any reason to wait? I'm happy to help move this forward if we agree that this is the right thing to do now. I'm concerned that it will become more difficult to make the changes and dedupe shared code the longer we wait.

@mortent
Copy link
Contributor

mortent commented Feb 28, 2023

I think the structure I suggest previously is a good start, although we already have a two additional modules for rollouts and healthchecks.

I think there is also possible that the kpt-core module here will be a bit of a catch-all. In particular we should probably try to avoid ending up with lots of dependencies on cluster clients libraries here, which can easily happen if we move code shared between controllers into it. It might be that we need a separate module for code shared between controllers, primarily rollouts and porch controllers (maybe these could all be in the same module?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devops enhancement New feature or request triaged Issue has been triaged by adding an `area/` label
Projects
None yet
Development

No branches or pull requests

3 participants