Add Event v2 package #5558
Add Event v2 package #5558
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5558 +/- ##
==========================================
- Coverage 71.05% 71.03% -0.02%
==========================================
Files 402 404 +2
Lines 15096 15401 +305
==========================================
+ Hits 10726 10940 +214
- Misses 3577 3661 +84
- Partials 793 800 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
i'm a little concerned by how much code has been directly copied over from the v1 version of these protos. is there a way we can abstract away some of this logic to be shared across packages? maybe by defining some of these methods on interfaces and having the v1 and v2 versions implement them?
| // ActionableErr returns an actionable error message with suggestions | ||
| func ActionableErrV2(phase Phase, err error) *protoV2.ActionableErr { | ||
| errCode, suggestions := getErrorCodeFromError(phase, err) | ||
| suggestionsV2 := make([]*protoV2.Suggestion, len(suggestions)) |
There was a problem hiding this comment.
do these need to be pointers? can we pass them by value instead
There was a problem hiding this comment.
Do you think that's a change we should make for v2? I mostly just did pointers since that's what the existing type uses
|
@nkubala I'm also concerned about it, I considered the route of creating interfaces for each of the proto types and simply passing them around like that, but I wasn't sure if that would also add unnecessary bloat since we'd need to add an interface for each of the proto types. |
Co-authored-by: Nick Kubala <[email protected]>
There was a problem hiding this comment.
discussed out of band, and this change should be fine. it's difficult in practice to squeeze to versioned protos into a single interface since the actual methods on the proto structs are autogenerated from the service definitions, and since we're deprecating the v1 version of the API, we'll likely remove the code at some point in the future eliminating the duplication.
| ev.logEvent(event) | ||
| } | ||
|
|
||
| // SaveEventsToFile saves the current event log to the filepath provided |
There was a problem hiding this comment.
This function can be moved to event/util package and re-used across both v1 and v2 proto
|
|
||
| state proto.State | ||
| stateLock sync.Mutex | ||
| eventChan chan *proto.Event |
There was a problem hiding this comment.
Can we change this to interface? Similar to
type listener struct {
callback func(interface{} error
errors chan error
closed bool
}
There was a problem hiding this comment.
we could try and save duplicate codes for func
ForEachEvent, Handle, logEvent, forEachEvent
We can spend some time seeing and how much code duplication it can save us.
What i am thinking is
- We have a broadcaster and a broadcasting channel which does not care about the type ie. v1, v2 proto but only emits whatever is sent to it.
This might need some complex refactoring.
Related: #5368 , #5422
Merge After: #5557 - this PR relies on the changes from #5557. The changes actually made in this PR are to
pkg/skaffold/event/v2/*.gopkg/skaffold/errors/errors.goDescription
Adds
pkg/skaffold/event/v2package. Almost all of the content is copied from the originalpkg/skaffold/eventpackage, with a couple new functions to handleTaskEventprotos (TaskFailed(),TaskInProgress(), andTestTaskFailed()).I've also updated the
pkg/skaffold/errorspackage to have a new function that returns an actionable error type for v2.