Embed labelling into Deployers #1463
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
+ Coverage 44.8% 45.11% +0.31%
==========================================
Files 112 112
Lines 4638 4637 -1
==========================================
+ Hits 2078 2092 +14
+ Misses 2350 2334 -16
- Partials 210 211 +1
Continue to review full report at Codecov.
|
| @@ -38,7 +37,7 @@ type Deployer interface { | |||
|
|
|||
| // Deploy should ensure that the build results are deployed to the Kubernetes | |||
| // cluster. | |||
| Deploy(context.Context, io.Writer, []build.Artifact) ([]Artifact, error) | |||
| Deploy(context.Context, io.Writer, []build.Artifact, []Labeller) error | |||
There was a problem hiding this comment.
instead of passing the labellers as a parameter to the Deploy() call, could we just embed the labellers themselves into the deployer object when we instantiate the deployer in the runner? imo the simpler the method signature the better, especially for something that might be exposed externally (for plugins) in the future
There was a problem hiding this comment.
I tried to go that way first but I felt like labelling is really part of the Deployer's contract so should be visible here.
There was a problem hiding this comment.
mmm ok I see your reasoning. sgtm
dgageot
force-pushed
the
embed-labelling-deployers
branch
from
baa6cfc to
f1683e6
Compare
This makes it easier to implement <> labelling strategies.
dgageot
force-pushed
the
embed-labelling-deployers
branch
from
f1683e6 to
dee1450
Compare
This makes it easier to implement <> labelling strategies.