kpt icon indicating copy to clipboard operation
kpt copied to clipboard

simplify package/dir structure in `kpt CLI` codebase

Open droot opened this issue 2 years ago • 4 comments

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 avatar Jun 09 '22 16:06 droot

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.

mortent avatar Jun 09 '22 18:06 mortent

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

droot avatar Jun 09 '22 19:06 droot

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.

natasha41575 avatar Feb 24 '23 20:02 natasha41575

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

mortent avatar Feb 28 '23 22:02 mortent