Add plugin context sharing types #127
Conversation
knative-prow
bot
added
approved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
=======================================
Coverage 81.10% 81.10%
=======================================
Files 69 69
Lines 5466 5466
=======================================
Hits 4433 4433
Misses 796 796
Partials 237 237 ☔ View full report in Codecov by Sentry. |
pkg/plugin/context_sharing_types.go
Outdated
| GetContextData() map[string]string | ||
|
|
||
| // ExecuteWithContext | ||
| ExecuteWithContext(ctx map[string]string, args []string) error |
There was a problem hiding this comment.
Just reading this, and I wonder whether we shouldn't use just a regular Golang context, alone to avoid confusion ? (at the end this is also only a map)
At least it should be then called ExecuteWithContextData but at the end I would move to Context instead of ContextData in the the whole interface definitions.
There was a problem hiding this comment.
We opted for contextData in a map directly. Basically I've refactored the interface from Context to plain map. :)
There was a problem hiding this comment.
It's a bit more straight-forward to work with map, rather than context.Context, than init context and add values.
There was a problem hiding this comment.
i see, but then maybe just rename to "WithContextData" ?
|
thanks for the update! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, rhuss The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
Part of client PR.
/cc @rhuss