-
Notifications
You must be signed in to change notification settings - Fork 23
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
CFP-25694: Move cilium/cilium-cli code into cilium/cilium repository #9
Conversation
5eabd0d
to
6c53ab9
Compare
38244c0
to
dd6e490
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... i have another radical idea. let me try and see if it works 🧠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
### 3. Release `cilium-cli` from [cilium/cilium-cli] repository We do not need to change the release process / cadence to accomplish the goals defined in this CFP. Continue releasing `cilium-cli` from [cilium/cilium-cli] at its own cadence using its own versioning that is independent of [cilium/cilium] versioning as you have been doing in [cilium/cilium-cli] repository. See [michi-covalent/cilium-cli] as an example of how [cilium/cilium-cli] repo might look like after the code migration. Basically it contains: - [`go.mod`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/go.mod) - A simple [`main.go`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/main.go) that calls [`NewCiliumCommand`](https://github.com/cilium/cilium-cli/blob/44ae1874fae4544c0db34dac89c11e37365b76ef/cli/cmd.go#L27) - Release [tags](https://github.com/cilium/cilium-cli/tags) and [artifacts](https://github.com/cilium/cilium-cli/releases) design-cfps/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md
Lines 84 to 93 in bf48480
### Impact: Module name change This proposal impacts users who depend on [cilium/cilium-cli] as a library. - The module path needs to be updated from `github.com/cilium/cilium-cli` to `github.com/cilium/cilium/cli`. - Instead of depending on a version of `github.com/cilium/cilium-cli` (`v0.14.5` for example), you need to inspect `go.mod` of a specific `github.com/cilium/cilium-cli` version, and transitively find the `github.com/cilium/cilium/cli` version to depend on.
i'm fine with either of these approaches. i'll document both 📝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... on the second thought, i like your approach better. let me just document your approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😹
b75de19
to
9f78360
Compare
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
9f78360
to
bf48480
Compare
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
c4d8603
to
64b82c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Replace io.ReadAll with safeio.ReadAllLimit so that custom-vet-check.sh [^1] doesn't complain when cilium-cli gets merged with cilium repo [^2]. For Cilium pod logs, set the limit to 1GB. 1GB of Cilium pod logs ought to be enough for anybody. [^1]: https://github.com/cilium/cilium/blob/main/contrib/scripts/custom-vet-check.sh [^2]: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Add errorlint linter to be consistent with cilium/cilium configuration [^1], and fix lint errors. Ref: cilium/design-cfps#9 [^1]: https://github.com/cilium/cilium/blob/3fa34b170493b48ef7befedef72badbd05429b18/.golangci.yaml#L117 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Document that we are planning to merge Cilium CLI code into cilium/cilium repository. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Co-authored-by: Bill Mulligan <billmulligan516@gmail.com> Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Add go-mod-directory parameter to override the directory that contains go.mod in case go.mod is not in local-path. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Move SysdumpHooks and InitSysdumpFlags to the sysdump package so that they can be imported from outside the cilium-cli repo. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Move the CLI version string from internal/cli/cmd to default/ so that it can be exported from outside the cilium-cli repo. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Store namespace and k8sClient as command context values so that the connectivity package can access them from outside the cli package once it moves to cilium/cilium repo. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Move Hooks to the api package so that the connectivity package can use the Hooks without creating a circular dependency once it moves to cilium/cilium repo. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Move the logging package out of internal/ so that it can be imported from outside cilium/cilium-cli repo. This is useful for building the cilium CLI binary from cilium/cilium repo [^1]. [^1]: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] 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]: cilium/cilium-cli#2449 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Install cloud CLIs so that you can run cilium-cli from inside a container and connect to AKS / EKS / GKE clusters. This commit also changes in-cluster scripts to use bash instead of sh. Some of these scripts are not POSIX-compliant, and they don't work with sh that comes with Ubuntu. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Add image-repo and image-tag parameters to the cilium-cli action that set up cilium-cli to run inside a container. Update aks-byocni.yaml to run cilium-cli inside a container using the action instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Update gke.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Update externalworklads.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Update {eks,eks-tunnel}.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Update multicluster.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Remove gopkg.in/check.v1 dependency in preparation to merge cilium-cli repo with cilium repo [^1]. Use github.com/stretchr/testify/assert for assertions instead. Ref: cilium/design-cfps#9 [^1]: https://github.com/cilium/cilium/blob/cd8f44c0e95f53f6e5cdbd6daffc5e9a3009c8a7/.golangci.yaml#L75 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Replace sync package with github.com/cilium/cilium/pkg/lock package so that lock-check.sh [^1] doesn't complain when cilium-cli gets merged with cilium repo [^2]. [^1]: https://github.com/cilium/cilium/blob/main/contrib/scripts/lock-check.sh [^2]: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Replace timer.After with inctimer.After so that custom-vet-check.sh [^1] doesn't complain when cilium-cli gets merged with cilium repo [^2]. [^1]: https://github.com/cilium/cilium/blob/main/contrib/scripts/custom-vet-check.sh [^2]: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Replace io.ReadAll with safeio.ReadAllLimit so that custom-vet-check.sh [^1] doesn't complain when cilium-cli gets merged with cilium repo [^2]. For Cilium pod logs, set the limit to 1GB. 1GB of Cilium pod logs ought to be enough for anybody. [^1]: https://github.com/cilium/cilium/blob/main/contrib/scripts/custom-vet-check.sh [^2]: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Add errorlint linter to be consistent with cilium/cilium configuration [^1], and fix lint errors. Ref: cilium/design-cfps#9 [^1]: https://github.com/cilium/cilium/blob/3fa34b170493b48ef7befedef72badbd05429b18/.golangci.yaml#L117 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
[ cherry-picked from cilium/cilium-cli repository ] Document that we are planning to merge Cilium CLI code into cilium/cilium repository. Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@brb it would mean a lot to me if you approve this pull request 🥦 cilium/cilium#34178 just got merged 🚀🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
cilium/cilium#25694