Proposal: Create a dedicated repository knative/client-pkg for shared code

[rhuss]

Feature request

I suggest creating a new repository in core knative called client-pkg which holds all the shared code that is proposed in #889

Use case

The motivation for this change is that this split-off is needed to avoid cyclic dependencies when e.g. plugin code is referring to client support libraries like parsing options in a common way or reusing the way how sinks can be specified.

At the moment those plugins have a dependency back to knative/client which is not only an issue from an architectural perspective but has real issues when it comes to inlining of plugins.

[rhuss]

An alternative option would be to include the shared code to knative/pkg, but that would introduce also some client specific dependencies like cobra to knative/pkg

[rhuss]

Let's revive this issue, to resolve cyclic dependencies. There are several options

• Fine granular for plugins only (knative-sandbox/kn-plugin-pkg and knative-sandbox/kn-plugin-source-pkg)
• One repo for all shared plugin libraries
• One repo knative/client-pkg for shared code among plugins, but also the shared code with the client itself (server access, option parsing, …) (as discussed in this issue)

Only the last option would have the benefit to break the dependency cycle (kn -> plugins (call dependency, but when inlined also code dependency) and plugins -> kn (for using the API introduced by the client)

This cycle can be resolved in two ways:

1. On a repository level as suggested by introducing knative/client-pkg
2. On a package level marking one directory as "shared" and all package within this directory are supposed to be exported. This would happen in the knative/client repository.

Option 1 has the benefit of clear separation as well as self-serving documentation that this is about shared code. The expectation is that people will be very careful when changing any signature within those packages. It would be on the same level as knative/pkg but for the client-side only. The drawback is (as @duglin mentioned in the client wg call) that there might be features that would touch both repositories. To sync those requires extra efforts (two PR that needs to be merged in a certain order)

Option 2 has the benefit that features are applied only to a single repo and also the release process is transactional (the mono-repo advantage). The drawback is that it is easy to overlook the external dependants that depend on an internal package. Also it does not resolve the cycle on the repository level.

Such a shared repository knative/client-pkg would hold:

• The client API for the interaction with serving and eventing. This can be reused not only on by plugins but by everyone who wants to benefit from additional features, like waiting on a service readiness
• Options parsing, both low-level (binary, list, maps) and high-level for shared options (--sink, ....)

My suggestion is to move on with knative/client-pkg. This decision is open for discussion so let's decide this together.
If we get a consensus until the end of next week, that would be perfect.

[github-actions]

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.

[rhuss]

/remove-lifecycle stale

[dsimansk]

Following the original knative/client-pkg that requires a new repository, There was another discussion around introducing submodule to original knative/client repository. I'd like to share a couple of points from my PoC.

Related docs:

• https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories
• https://github.com/go-modules-by-example/index/tree/master/009_submodules

PoC changes:

• https://github.com/dsimansk/client/tree/public/v0.0.1
• https://github.com/dsimansk/kn-plugin-source-kafka/tree/pr/multimodule
• knative-extensions/kn-plugin-source-kafka@main...dsimansk:pr/multimodule

Repository structure

The knative.dev/client/public layout. So far I've only moved 2 modules to test it out. However, the end game should be to move everything that's shared with public, currently everything that's shared with kn's plugins.
There's no local vendor/ dir on purpose, but our current update-deps.sh scripts will need to be updated to make it work smoothly and skip vendoring.

➜  public git:(pr/multimodule) tree ./    
./
├── errors
│   ├── errors.go
│   ├── errors_test.go
│   ├── factory.go
│   ├── factory_test.go
│   ├── knerror.go
│   ├── knerror_test.go
│   └── types.go
├── go.mod
├── go.sum
├── LICENSE
└── plugin
    ├── manager.go
    ├── manager_test.go
    ├── stat.go
    ├── stat_windows.go
    ├── verify.go
    └── verify_test.go

2 directories, 16 files

Using the modules

To consume the public module back in existing client code. I've used the following hack(?). Consequently, the mechanics of go mod vendor will always copy the original REPO_ROOT_DIR/public to REPO_ROOT_DIR/knative.dev/client/public. Effectively creating a duplicate, see.

knative/client/go.mod file entry

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

For the plugins, the current PoC has both client and client/public dependencies, but ideally we'd move everything shared to client/public.

https://github.com/dsimansk/kn-plugin-source-kafka/blob/pr/multimodule/go.mod#L21-L23

Versioning

The final point to consider is versioning and release process. For the public module to be resolvable by go mod queries the repository needs to be tagged.

That's seems like the most significant drawback, as we'd need to handle this special case and create seconday tag for public module, here.

E.g. for the next release v0.24.0, there'd 2 tags on knative/client repo.

v0.24.0
public/v0.24.0

[evankanderson]

Just to clarify the TOC concerns:

• The release (as described in https://github.com/knative/release) is currently a 7-stage process due to dependencies between repos. There's also a certain ongoing cost syncing actions, groups, etc between repos. As we untangle dependencies (good!), we should avoid simply moving the mess to the release process, and instead see if we can simplify outcomes.
• It might be worth considering whether this code could live in a subdirectory in pkg. I'd need to test how go mod handles pruning dependencies which aren't used so that (for example), queue-proxy in serving doesn't end up linking in Cobra and other CLI tools by accident. (They can still do it on purpose if they actually want it.)

[dsimansk]

I like the idea of reusing existing knative.dev/pkg. I've check a few plugins and the code that's currently shared could be moved pkg without too much interference. Mainly the shared code includes utils, common flags, plugins and printers packages.

However, if we'd like to share kn's client facades then we could start to hit the actual limitations.
@rhuss wdyt about starting incrementally with pkg as shared code location. E.g. I can modify my PoC to remove direct client dependency in kn-plugin-source-kafka and substitute with knative.dev/pkg only.

[rhuss]

@dsimansk Would this be a submodule in knative.dev/pkg (with an own go.mod) or just added as packages ? We considered the latter already but that would introduce some client-only dependencies (cobra, pflag, viper, go-homedir, x/term, ..., and there transitive dependencies) which would be added to pkg's vendor/ dir, but is not useful for server-side projects that depend on pkg (and that are many server-side projects).

If this is not an issue, I'm fine with adding it to the general pkg, but I'm afraid we are then one step further to create a knative kitchen sink (a bit like the symptom of a global "util" package that every larger program project has and that contains some random stuff and introduces dependency headaches over the long run).

[rhuss]

A JavaEE analogy would be to add AWT or Swing widgets to a server-side WAR dependency.

[dsimansk]

Sure, you're right @rhuss. Surprisingly I've kind of mixed knative.dev/pkg and knative.dev/test-infra that has cobra dependencies already. In fact we'd introduce a few CLI dependencies that aren't required elsewhere.

The Evan's comments mentioned that concern as well. Indeed we need to find out if it pollutes server-side repositories as well.

It might be worth considering whether this code could live in a subdirectory in pkg. I'd need to test how go mod handles pruning dependencies which aren't used so that (for example), queue-proxy in serving doesn't end up linking in Cobra and other CLI tools by accident. (They can still do it on purpose if they actually want it.)

[dsimansk]

Sorry for the delay, I'd like to share a bit more points to keep this issue moving.

knative.dev/pkg example with less dependent plugin:

• dsimansk/pkg@f621c11
  • modified pkg with a bit of shared kn's code
• dsimansk/serving@b13ada9
  • Serving repo is not pulling any new kn's dependencies like cobra
• dsimansk/kn-plugin-source-kafka@b0a3f21

It wouldn't quiet straightforward with plugin like func that relies directly kn's client facade to access/modify Knative resources. (shared scope with kn actually).

• https://github.com/boson-project/func/blob/main/knative/deployer.go#L45-L52

To summarize the above, there was an concern that using pkg would have a adverse side-effect to controller components/repos. That's addressed in the first example. It'd be possible to share code through pkg safely.

The second code pointer to func exposes the other concern. The kn repo might have code that's directly depending on knative.dev/serving or knative.dev/eventing. And still it's useful to be used with plugin as well. However, I believe that couldn't be moved to pkg as it'd be breaking the concept of having place for shared code and breaking dependency cycles. (no Serving or Eventing dependencies are currently present in pkg).

In conclusion, it seems that best possible solution to address kn's shared code space is a standalone knative.dev/client-pkg repository. There's currently at least on high profile consumer (func plugin) that'd not be resolved with only pkg shared location. IMO, the two main needs are flexibility to adds kn specific deps and then well-known release process (one module per repository, automation, jobs etc.).

/cc @rhuss updated PTAL

[rhuss]

Let me try to summarize (and verify that I understand correctly):

• Putting the client shared code as a subdirectory to knative.dev/pkg is not possible because the client-pkg has dependencies on eventing and serving that can be reused by plugins (i.e. the client's simplified access API that e.g. includes sync waiting on services)
• Putting the client shared code as go submodule in knative.dev/client is difficult because of some hacky go.mod replacements and multiple tags required.
• Having a dedicated knative.dev/client-pkg would be the most straight forward part, with the drawback to add to the release burden another dependency.

If this is the case I see two options:

• Either avoid any eventing and serving dependencies in the client-shared code. In this case plugins would need to depend directly on knative.dev/client for accessing the client API
• Or create a knative.dev/client-pkg as originally requested.

Is this kind of a fair summary @dsimansk ?

[dsimansk]

Yes, that's perfect summary @rhuss.

Either avoid any eventing and serving dependencies in the client-shared code. In this case plugins would need to depend directly on knative.dev/client for accessing the client API

We can definitely start to move those parts with out Serving,Eventing dependency. And re-evaluate the need for standalone client-pkg repo in a while?

[evankanderson]

We don't see a good alternative that's not a lot of work, so let's do this for now and create the dependency chain:

hack -> pkg -> {serving, eventing} -> client-pkg -> client
hack -> pkg -> {serving, eventing} -> client-pkg -> kn-plugin-*

Whereas today it is:

hack -> pkg -> {serving, eventing} -> client -> kn-plugin-*

[evankanderson]

(And at some point, it might be worth versioning the serving/eventing APIs / generated code separately from the rest of those repos)

[dprotaso]

+1 in favour of adding the repo

[rhuss]

when doing the refactoring, here some ideas how to structure the top-level directory in client-pkg --> #889 (comment)

[evankanderson]

/close

[knative-prow-robot]

@evankanderson: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.