Proposal: Fold back in client-pkg to Go submodule

[cardil]

Summary

Move back the client-pkg code to this repository, and create pkg Go submodule, that could be consumed by plugins or wrappers.

Problem

Currently, after extracting the client-pkg repository (see: https://github.com/knative/client/issues/1039), but not removing the code in the client, we end up with duplicated code both here and in the client-pkg. Most of the code is actually newer here, but plugins consume client-pkg. Of course, that's messy.

I tried to fix that in these PRs:

• https://github.com/knative/client/pull/1935
• https://github.com/knative/client-pkg/pull/177

These PRs effectively removed most of the code from the client in favor of the client-pkg. That made me realize, though, that it's probably going to make contributing to the project more cumbersome. Almost any change will need to be done in client-pkg, and then after the merge, the library reference in the client repo will need to be updated. Most likely in most cases, such a change would also require some refactoring in the client, so the automated auto-bump PRs will not be enough.

Execution

We can reconcile the two versions of the code (here and in the client-pkg) by choosing the more modern one and putting it back here, in the client repo. At the same time, deprecate the client-pkg repo. To separate the code and allow plugins to use only the public, library part of this client, we could create a Go submodule for the pkg/ directory here. The non-public code of the client can be moved to the internal/ directory (following Go's suggested project structure). Also, by adding a go.work file, we can effectively make contributing to the client transparent (our tooling already supports this well).

The structure would look as follows:

knative.dev/client/
├── go.mod
├── go.work
├── cmd/
├── internal/
└── pkg/
    ├── go.mod
    └── vendor/

For the code in the main module, we need a relative replace stanza in the main go.mod file:

replace knative.dev/client/pkg => ./pkg

The go.work file, and a separate go modules for the knative.dev/client package and knative.dev/client/pkg wouldn't be required, if we didn't use embedding of the plugins, in the downstream projects.

Validation

I exercised such a repository structure in the following projects:

• github.com/cardil/knative-multimodule-release-check
• github.com/cardil/knative-multimodule-consume

I was able to successfully release a multi-module repository using the Knative tools. I only had one minor hiccup, but it was not related to the structure, as I was releasing locally, not on the Prow. All of our tools, such as update-deps, testing work as well.

The consuming of such multi-module sub package, was also not a problem. One minor inconvenience was the versioning. In Go, the tag only works just for the main module, as @dsimansk mentioned in https://github.com/knative/client/issues/1039#issuecomment-852189944. Still, our tools work well with it. The update deps tool, the buoy, can float submodules based on the release branch. And we don't currently tag the client package, so very little changes for the consumer.

If we really want the tags to work as well for the pkg/ submodule, we could double-tag each release in this way:

v0.33.0
pkg/v0.33.0

Alternatively, maybe we could set up some redirects in a similar way as we support the knative.dev domain for go imports.

But personally, I think this is not necessary, as even without tags on the pkg/ submodule, we are still on par with the current way of working.

The only real drawback to the proposed structure would be breaking Go's remote run feature for kn. Currently, people can run kn from anywhere Go is installed:

$ go run knative.dev/client/cmd/kn@latest

And that will download and execute the client. This breaks by adding the replace stanza in the main go.mod file. The go install breaks as well:

$ go run knative.dev/client/cmd/kn@latest              
go: downloading knative.dev/client v0.33.0
go: knative.dev/client/cmd/kn@latest (in knative.dev/client@v0.33.0):
    The go.mod file for the module providing named packages contains one or
    more replace directives. It must not contain directives that would cause
    it to be interpreted differently than if it were the main module.

We don't mention this support in any documentation or tutorials, but it's still a bit of a bummer. Personally, I think it's a bug in Golang, because why wouldn't Go support multi-module projects in this way?

Alternatives

Embed plugins in a dedicated repo

We embed the plugins in kn, downstream. If we create a dedicated repo just for this, say kn-blundle, we could avoid adding multiple go modules (and workspace file) as there will be no cycles.

/kind proposal

[rhuss]

I'm not generally opposed to the proposal (aside from the fact that I constantly had issues with git submodules in various toolings like IDEA, so I really try to avoid them, as they are also very confusing for Git newbies), but I think we should also revisit the original motivation.

The main reason for introducing client-pkg is to break cyclic dependencies between client and plugins on a repository level. Since Knative still sticks to a multi-repository approach backed by tons of automation (instead of a mono-repo), this repository separation makes it easy to understand the dependencies.

The main problem is this:

... but not removing the code in the client, we end up with duplicated code both here and in the client-pkg

IMO, it should be mandatory to remove the code in client when it has been moved to client-pkg. I know this will break older plugins, but it is also a good way to determine which plugins are still maintained/used. We can pin their dependencies to an older version of the client.

From my feeling, Git submodules make everything even more complex than it already is and even harder to understand for people new to the project or people coming back to the project after some time.

But if we decide to go this route, I would only accept this change with an extensive developer documentation update explaining how to set up a project like this.

@cardil, do you have some samples of other open-source projects that use a similar setup to keep cyclic dependencies on the repository level but logically break them with git submodules? Or do we invent something new here? I would be eager to learn from some past experiences with this usage of git submodules first.

[rhuss]

BTW, I'm not alone in my suspicion against git submodules. There are quite some strong opinions out there, too:

• https://news.ycombinator.com/item?id=31792303
• https://codingkilledthecat.wordpress.com/2012/04/28/why-your-company-shouldnt-use-git-submodules/
• https://diziet.dreamwidth.org/14666.html
• https://www.feoh.org/posts/git-submodules-are-awful-but-occasionally-necessary.html
• ....

[cardil]

@rhuss: BTW, I'm not alone in my suspicion against git submodules. There are quite some strong opinions out there, too: [..]

Roland, this has nothing to do with GIT submodules. It's about having multiple GO modules (and a GO workspace file) in one GIT repository.

[rhuss]

🤦 Yeah, I totally messed this up, shame on me .... Forget my last comment.

Still, my question would be if there has been some prior art so that we can learn from their experiences.

[cardil]

Kubernetes has recently adapted go workspace with multiple go modules: https://github.com/kubernetes/kubernetes/blob/d83cd48e5ebbb1b073164a574ef9aa5a68569d9c/go.work

[rhuss]

I'm ok with this if other are ok, too.

[dsimansk]

Per several talks with @cardil on the topic. I'm definitely +1 to move forward.

[lkingland]

This seems fine to me. ACKing because it may require a small update to Functions.

[cardil]

Okay. I'll proceed with the PRs...

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[cardil]

/remove-lifecycle stale /triage accepted