-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Adjust to changes in listener/pkg/v2
#2697
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
feat: Adjust to changes in listener/pkg/v2
#2697
Conversation
a8aff69
to
0b013a4
Compare
listener/pkg/v2
67bdff3
to
0b013a4
Compare
04e9caa
to
9e388ac
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.
LGTM. One question about the watcherEventAdapter
. Is it temporary until things are made pretty or going to stay? Should we add the internal/controller
pkg to the unit test coverage thresholds?
And I really appreciate your detailed PR descriptions and change summaries. Made it super easy to follow through the preceding changes in runtime-watcher leading up to this PR 👍🏻
CC: @jacekon
We can do this, yes. But it has 100% now, so it can only get worse over time :) |
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.
We can do this, yes. But it has 100% now, so it can only get worse over time :)
That's the point of the action 😄
But seriously - I think it is sort-of temporary and will be replaced by a bigger refactoring like the one introduced in #2657
Feel free to suggest a "better place" (a dedicated package?) for this thing - until the "big refactoring" comes.
No it is okay for me already as indicated by my previous approval. Especially now since you mentioned that this will be refactored soon anyway.
1d43296
into
kyma-project:main
Description
This PR is meant as a prerequisite to #2657 , that is: I think we should merge this PR first.
The #2657 is much more complex and it's not clear if this additional complexity is worth it.
Besides that, the #2657 is doing two things at once: It tries to adapt
lifecycle-manager
to the latest changes inruntime-watcher/listener
(which is necessary) and it refactors the event-handling logic (which is not strictly required to accomplish kyma-project/runtime-watcher#130)In my opinion these two things should NOT be delivered in a single PR as it makes code review unnecessary hard.
Considering that, in order to follow the good practice:
first: make it work, then make it pretty
, this PR introduces just the minimal changes required to make lifecycle-manager compatible with recent changes in theruntime-watcher/listener
module.Changes proposed in this pull request:
runtime-watcher/listener/pkg/v2
typesruntime-watcher:2.0.0
imageRelated issue(s)
kyma-project/runtime-watcher#130