CFP-25694: Move cilium/cilium-cli code into cilium/cilium repository#9
CFP-25694: Move cilium/cilium-cli code into cilium/cilium repository#9michi-covalent merged 5 commits intomainfrom
Conversation
michi-covalent
force-pushed
the
pr/michi/25694
branch
from
5eabd0d to
6c53ab9
Compare
michi-covalent
force-pushed
the
pr/michi/25694
branch
7 times, most recently
from
38244c0 to
dd6e490
Compare
gandro
left a comment
gandro
left a comment
There was a problem hiding this comment.
I know this is still in draft, but I've left a bit of early feedback
|
|
||
| We need to come up with a git tag convention for cilium-cli releases to distinguish | ||
| tags for Cilium releases from tags for cilium-cli releases. My proposal to use | ||
| `cilium-cli/vX.Y.Z` for cilium-cli release tags. |
There was a problem hiding this comment.
Whatever scheme we come up with, this needs to be compatible with Go modules, otherwise it will break users who pull in cilium/cilium or cilium/cilium-cli as a go module.
If we want to merge merge cilium-cli into the github.com/cilium/cilium Go module, then pulling in cilium-cli as a dependency will be tricky, because then the version of the Go module would inherently be tied to Cilium (the CNI) releases, because Go mod requires semantic versioning, and cli-v0.13.1 is not a valid semantic version.
If, on the other hand, we want to keep cilium-cli as it's own Go module, and it's import path would be e.g. github.com/cilium/cilium/cli then we'll need something like cli/vX.Y.Z according to https://go.dev/blog/publishing-go-modules - but that means we'd still a dependency from the CLI module to the CNI module. That might require us to use Go workspaces.
I think this point (separate Go module vs. shared Go module) should be a key question that needs to be answered as part of the CFP.
There was a problem hiding this comment.
this is an excellent point, we probably need to go with the latter option (cilium-cli as a separate go modules) to continue supporting importing cilium/cilium-cli as a go module.
but that means we'd still a dependency from the CLI module to the CNI module. That might require us to use Go workspaces.
yeah makes sense to explore using go workspaces, instead of doing replace github.com/cilium/cilium => ../ 🙀
There was a problem hiding this comment.
... i have another radical idea. let me try and see if it works 🧠
There was a problem hiding this comment.
Another option might be to use an approach similar to what upstream k8s is doing for some of its modules, e.g. k8s.io/apimachinery where code changes are made in the main k8s repo staging directory (i.e. https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/apimachinery in case of k8s.io/apimachinery) and these are then synced to their own repo (i.e. https://github.com/kubernetes/apimachinery in case of k8s.io/apimachinery). More details at https://github.com/kubernetes/kubernetes/blob/master/staging/README.md
I think using a similar approach for cilium-cli would allow to use the in-tree version for cilium/cilium CI while still allowing independent releases from cilium/cilium-cli without having to keep e.g. multiple go.mod files in cilium/cilium or using Go workspaces.
There was a problem hiding this comment.
yeah i think this is the cleanest way to avoid impacting users who import github.com/cilium/cilium-cli as a module.
i was thinking about a similar, but much lazier approach:
design-cfps/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md
Lines 57 to 70 in bf48480
design-cfps/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md
Lines 84 to 93 in bf48480
i'm fine with either of these approaches. i'll document both 📝
There was a problem hiding this comment.
... on the second thought, i like your approach better. let me just document your approach.
There was a problem hiding this comment.
... i'm having third thought 😹 if we take this approach, where does go.mod file for cilium-cli come from? i see that these staging directories contain go.mod files with some replace directives. for example: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/go.mod
There was a problem hiding this comment.
Together with @gandro we've looked a bit into how synchronizing the staging directories to the standalone repos works. It seems that go.mod is synched, but the replace directive is removed as part of the merge commit in the standalone repo (?), see kubernetes/apimachinery@b438789 for an example.
We've also come up with the idea of not keeping a separate go.mod for the in-tree cilium-cli at all and just use the main go.mod. Dependencies of cilium-cli would then be kept there as well. The main go.mod is then copied to the cilium/cilium-cli on sync and go mod tidy && go mod vendor is run. This should prune all the dependencies that are only used by cilium the CNI, but not cilium-cli.
Haven't tested the approach, but this might be something else to consider.
There was a problem hiding this comment.
not keeping a separate go.mod for the in-tree cilium-cli at all and just use the main go.mod.
yeah i don't think keeping cilium-cli as a separate module is worth all the effort.
however, with this approach, you also need to modify all the import paths in .go files back from github.com/cilium/cilium/cli to github.com/cilium/cilium-cli.
... i'm starting to like my original proposal again 😹
xmulligan
changed the title
michi-covalent
force-pushed
the
pr/michi/25694
branch
12 times, most recently
from
b75de19 to
9f78360
Compare
Signed-off-by: Michi Mutsuzaki <[email protected]>
michi-covalent
force-pushed
the
pr/michi/25694
branch
from
9f78360 to
bf48480
Compare
Signed-off-by: Michi Mutsuzaki <[email protected]>
michi-covalent
force-pushed
the
pr/michi/25694
branch
from
c4d8603 to
64b82c0
Compare
There was a problem hiding this comment.
It seems to me that the main disconnect is that we want both
- codevelopment of feature (in cilium/cilium) and test (currently in cilium-cli),
- but also the benefits of isolating cilium-cli from the cilium codebase (for release cadence, dev velocity, ensuring compatability, ...)
Or put differently, is cilium-cli a tool primarily for users, or primarily for CI?
Should we be instead working towards a future where we can develop tests in cilium/cilium, and in CI the CLI picks them up and runs them against the PR version?
i'm not too worried about release cadence and ensuring compatibility. i believe this CFP addresses these points reasonably well. dev velocity is a real concern. however, once we finish migrating cilium-cli to helm mode, most of the cilium-cli development might become:
for these things, it might actually be faster to develop everything in cilium/cilium 🤔 merging two repos will most likely reduce the overall number of dependency update pull requests too. i guess what i'm saying is: dev velocity is a bit difficult to quantify 💭
potentially, i need a bit more detail on how this might look like. happy to chat about it. |
Signed-off-by: Michi Mutsuzaki <[email protected]>
Move the connectivity package from cilium/cilium-cli repo. The main goal here is to make "cilium connectivity test" use the local package so that you can develop new features and add tests for the new feature without having to deal with multiple repositories. There is a corresponding pull request in the cilium-cli repo [^1] to import the connectivity package from the cilium repo. See CFP-25694 [^2] for additional details. The following files are direct copies from [email protected] with some minor adjustments like renaming package paths and fixing lint errors: - cilium-cli/cli/connectivity.go [^3] - cilium-cli/connectivity/* [^4] - cilium-cli/cmd/cilium/main.go [^5] [^1]: cilium/cilium-cli#2355 [^2]: cilium/design-cfps#9 [^2]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cli/connectivity.go [^3]: https://github.com/cilium/cilium-cli/tree/v0.16.4/connectivity [^4]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cmd/cilium/main.go Signed-off-by: Michi Mutsuzaki <[email protected]>
Move the connectivity package from cilium/cilium-cli repo. The main goal here is to make "cilium connectivity test" use the local package so that you can develop new features and add tests for the new feature without having to deal with multiple repositories. There is a corresponding pull request in the cilium-cli repo [^1] to import the connectivity package from the cilium repo. See CFP-25694 [^2] for additional details. The following files are direct copies from [email protected] with some minor adjustments like renaming package paths and fixing lint errors: - cilium-cli/cli/connectivity.go [^3] - cilium-cli/connectivity/* [^4] - cilium-cli/cmd/cilium/main.go [^5] [^1]: cilium/cilium-cli#2355 [^2]: cilium/design-cfps#9 [^2]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cli/connectivity.go [^3]: https://github.com/cilium/cilium-cli/tree/v0.16.4/connectivity [^4]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cmd/cilium/main.go Signed-off-by: Michi Mutsuzaki <[email protected]>
Move the connectivity package from cilium/cilium-cli repo. The main goal here is to make "cilium connectivity test" use the local package so that you can develop new features and add tests for the new feature without having to deal with multiple repositories. There is a corresponding pull request in the cilium-cli repo [^1] to import the connectivity package from the cilium repo. See CFP-25694 [^2] for additional details. The following files are direct copies from [email protected] with some minor adjustments like renaming package paths and fixing lint errors: - cilium-cli/cli/connectivity.go [^3] - cilium-cli/connectivity/* [^4] - cilium-cli/cmd/cilium/main.go [^5] [^1]: cilium/cilium-cli#2355 [^2]: cilium/design-cfps#9 [^2]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cli/connectivity.go [^3]: https://github.com/cilium/cilium-cli/tree/v0.16.4/connectivity [^4]: https://github.com/cilium/cilium-cli/blob/v0.16.4/cmd/cilium/main.go Signed-off-by: Michi Mutsuzaki <[email protected]>
Replace the connectivity package with the one from cilium/cilium repo. The main rationale for moving the connectivity package to cilium/cilium repo is to enable Cilium developers to implement new features and add tests for the new features without having to deal with multiple repos. According to @brb, this will drastically boost Cilium developers' productivity, bringing the feature development velocity to the next level. See CFP-25694 [^1] for additional details. Renovate is configured to upgrade to cilium/cilium pre-releases [^2], so cilium-cli gets synced to the latest cilium/cilium connectivity package from the main branch roughly once every month. [^1]: cilium/design-cfps#9 [^2]: #2449 Signed-off-by: Michi Mutsuzaki <[email protected]>
|
@michi-covalent is this still up to date with the current ideas? |
|
yes coming soon i promise |
|
Ok, thanks. Was actually just checking if you think it is ready to merge or if anything else needs to be added or updated |
|
One thing I like from the proposed process in #37 (README preview) is this idea that we can merge a CFP once the ideas are agreed in principle, with the status being "implementable". If the CFP is up to date and the relevant stakeholders approve, we are good to do that even if we are still going through the practical ramifications of how we implement this. Then we can come back later to update the CFP as "experimental" / "stable" if something changes significantly in the design. Those changes would also be PRs and we can always debate the details at that point too. Merging an "implementable" design does not mean we are hard committed to the specific approach, things are always still flexible. We could trial this process with this PR if you like. |
|
I've tried to outline a short status framework here. Practically speaking, if this CFP has the approval of one committer, then it could be merged as implementable. |
Co-authored-by: Bill Mulligan <[email protected]> Signed-off-by: Michi Mutsuzaki <[email protected]>
|
@brb it would mean a lot to me if you approve this pull request 🥦 cilium/cilium#34178 just got merged 🚀🙏 |
8 participants
cilium/cilium#25694