cilium / design-cfps

Repo to store Cilium CFP design docs
Apache License 2.0
21 stars 19 forks source link

CFP-25694: Move cilium/cilium-cli code into cilium/cilium repository #9

Open michi-covalent opened 1 year ago

michi-covalent commented 1 year ago

https://github.com/cilium/cilium/issues/25694

michi-covalent commented 1 year ago

but also the benefits of isolating cilium-cli from the cilium codebase (for release cadence, dev velocity, ensuring compatability, ...)

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 💭

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?

potentially, i need a bit more detail on how this might look like. happy to chat about it.

joestringer commented 1 year ago

One more thought, I like how sysdump isn't too heavily tied to individual cilium versions, because cilium-cli delegates some of the details to Cilium. In the back of my mind, I'm wondering if the solution to this isn't to more tightly couple cilium & cilium-cli, but rather the inverse - further decouple the details so that cilium-cli needs fewer cilium-specific changes over time.

michi-covalent commented 1 year ago

thank you all for your feedback 🙏. let me try to summarize all the discussions here.

at a very high level, there are 2 options that emerged from this CFP:

based on the discussion in this thread, looks like people are ok with option B. since we have already discussed option A extensively, i'll focus on option B in this comment.

pros

cons / open questions

i'll write up a proposal for option B in a separate file 📝

michi-covalent commented 8 months ago

ok i've been sleeping on this for a while now, talking to various people here and there. let me summarize what i'm currently thinking, and what next steps we should take to achieve the original goal of minimizing the feature development overhead for adding new integration tests for cilium features.

tldr

pursue option B: use cilium-cli's Hook interface to add tests.

what's wrong with merging cilium-cli repo to cilium repo?

nothing is wrong per se, and merging cilium-cli repo is certainly one way to make it easier to use the connectivity package. however now i see flaws in my original reasoning for merging cilium-cli repo to cilium repo. my original reasoning went like this:

  1. i need a way to add integration tests in cilium/cilium.
  2. i like using cilium-cli's connectivity package for writing tests.
  3. however i don't like the connectivity package being in a different repository because i need to open a separate pull request if i need to enhance the connectivity package to accommodate what i need to accomplish.
  4. therefore let's merge cilium-cli repo into cilium repo.
  5. face any unpredictable consequences in terms of disrupting cilium-cli's development and release processes.
  6. figure out if we need to maintain cilium-cli's go module name to avoid breaking projects that import cilium-cli.

the main flaw of this reasoning is in 3 where it jumps from "i need to improve the package i'm using for testing" to "let's merge the entire repo to avoid opening separate pull requests for it". if it were any other testing library (like let's say testify), i'd be much more hesitant to copy it over into cilium repo just to avoid having to open separate pull requests.

i acknowledge that the connectivity package is still in a very early stage of development in terms of enabling outside projects to use it as a library. therefore early adopters often find that they need to improve the connectivity package. @jibi has been trailblazing and making many contributions in this area, including:

my naively optimistic assertion is that this situation will improve over time as the connectivity package matures, and we can get to the point where most people can simply use cilium-cli's Hook interface to add new tests without having to polish off rough edges in the connectivity package.

if this assertion is wrong, pain points for using the connectivity package should be easily identifiable. and again i'm optimistically confident in cilium community's ability to come up with pragmatic solutions to address these pain points.

next steps

michi-covalent commented 4 months ago

sorry for the delay here are proof of concept draft PRs:

we might decide to move more packages later on but i think this is a good first cut. @brb @tklauser please take a look and let me know if the overall approach looks ok 🚀🙏

brb commented 4 months ago

@michi-covalent Thanks! Could you TL;DR a new workflow when adding new feature / tests to cilium/cilium? In particular, how they will be run by CLI in the existing GH workflows?

michi-covalent commented 4 months ago

yeah so it uses cilium-cli github action https://github.com/cilium/cilium-cli/blob/main/action.yaml to build and install the Cilium CLI from pkg/cilium-cli directory with this main.go: https://github.com/cilium/cilium/blob/5a838dbb895ef3288c9a14f9e997b44bab874105/pkg/cilium-cli/main.go so you can add you tests in pkg/cilium-cli/connectivity/ and they'll magically get picked up in your PR 🪄