-
Notifications
You must be signed in to change notification settings - Fork 263
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
Proposal: Fold back in client-pkg to Go submodule #1941
Comments
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 The main problem is this:
IMO, it should be mandatory to remove the code in 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. |
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. |
🤦 Still, my question would be if there has been some prior art so that we can learn from their experiences. |
Kubernetes has recently adapted go workspace with multiple go modules: https://github.com/kubernetes/kubernetes/blob/d83cd48e5ebbb1b073164a574ef9aa5a68569d9c/go.work |
I'm ok with this if other are ok, too. |
Per several talks with @cardil on the topic. I'm definitely +1 to move forward. |
This seems fine to me. ACKing because it may require a small update to Functions. |
Okay. I'll proceed with the PRs... |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
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: #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:
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 theinternal/
directory (following Go's suggested project structure). Also, by adding ago.work
file, we can effectively make contributing to the client transparent (our tooling already supports this well).The structure would look as follows:
For the code in the main module, we need a relative replace stanza in the main
go.mod
file:The
go.work
file, and a separate go modules for theknative.dev/client
package andknative.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:
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 #1039 (comment). 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: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:
And that will download and execute the client. This breaks by adding the replace stanza in the main
go.mod
file. Thego install
breaks as well: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
The text was updated successfully, but these errors were encountered: