Conversation
7d9a222 to
40bf4d9
Compare
5954750 to
11fbb74
Compare
|
/test |
ad98280 to
8276d67
Compare
|
/test |
8276d67 to
f97f1d8
Compare
nebril
left a comment
There was a problem hiding this comment.
ci-structure LGTM. Good luck getting it merged!
c6e1b02 to
25261fc
Compare
|
/test |
gandro
left a comment
There was a problem hiding this comment.
Thanks so much for cleaning up the imports! I did find some instances in my CODEOWNERs where the imports cleanup went slightly wrong (I have not checked files not owned by me)
gandro
left a comment
There was a problem hiding this comment.
Awesome, this looks good to me now 🚀
|
/test |
chancez
left a comment
There was a problem hiding this comment.
Hubble related changes lgtm.
learnitall
left a comment
There was a problem hiding this comment.
Woohoo! 🥳. Quick clarifying question for you, but otherwise LGTM for API changes.
|
I did a quick sanity check and I think there are still some places where the old hive package is referenced. Were these left in on purpose? It looks like there are some leftover references in Documentation and comments, as well as some leftover imports to $ grep 'pkg/hive' -R . --include \*.go --include \*.rst | wc -l
89Examples: |
These are intentional. The PR's purpose is not to remove |
|
/test |
learnitall
left a comment
There was a problem hiding this comment.
Looks good, thank you! My apologies for the oversite there 🤦
The general idea is to have `pkg/hive` contain the cilium-specific
adaptations of `cilium/hive`. Specifically, that's providing certain
cells like statedb and jobs directly, and having module decorators
creating the right health and logging components.
It's one big commit as it has proven difficult to get intermediate
commits to compile (which our CI mandates). The health part needs a bit
of love after this, we can probably move `healthv2` into `hive/health`
as it's strongly tied to cilium's hive.
The bulk of the changes are mechanical. The categories are:
1. import path change from cilium/cilium/pkg/hive/{cell,job} to
cilium/hive/{cell,job}
2. giving hive.{Start,Stop,Run} an slog.Logger to provide to the hive,
3. changes to the health interfaces - Scope and HealthReporter are
unified under just Health
4. modules are provided with a job group scoped to their module id
directly, instead of having to create a group from a registry
In addition, a broken test is fixed in api_limits_test. The test was
inadvertently using global state of pflag, and reading the config did
_not_ actually work, but just inherited the existing state from a
previous test.
Finally, in pkg endpoint, due to the removal of cell.GetSubScope, a bit
of health logic is exposed - cell.GetSubScope used to allocate a new
shim scope if the passed parent is `nil`, which it is for EP tests.
Instead of fixing up all the tests which don't pass a health, put that
logic into pkg/endpoint where it's at least visible, and can be cleaned
up if so desired.
Signed-off-by: Jussi Maki <jussi@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
|
/test |
The general idea is to have
pkg/hivecontain the cilium-specific adaptations ofcilium/hive. Specifically, that's providing certain cells like statedb and jobs directly, and having module decorators creating the right health and logging components.It's one big commit as it has proven difficult to get intermediate commits to compile (which our CI mandates). The health part needs a bit of love after this, we can probably move
healthv2intohive/healthas it's strongly tied to cilium's hive.The bulk of the changes are mechanical. The categories are:
cilium/cilium/pkg/hive/{cell,job}tocilium/hive/{cell,job}hive.{Start,Stop,Run}anslog.Loggerto provide to the hive,ScopeandHealthReporterare unified under justHealth