Remove duplication in code handling labels #723
Conversation
pkg/skaffold/runner/label/label.go
Outdated
| } | ||
|
|
||
| // WithLabels creates a deployer that sets labels on deployed resources. | ||
| func WithLabels(d deploy.Deployer, labels map[string]string) deploy.Deployer { |
There was a problem hiding this comment.
Seems a little strange to me that this lives in the label package, could we have deploy.WithLabels(...) instead?
There was a problem hiding this comment.
I used to have with* in deploy and build and @r2d4 made me move them to runner. lol.
Either way make me happy.
There was a problem hiding this comment.
lol. I put the labels package under runner because it's what was kicking off the actual labeling before, but if we're going to have the labeller embedded in the deployer itself I think we should move it out. @r2d4 WDYT
pkg/skaffold/runner/runner.go
Outdated
| @@ -92,23 +72,18 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *config.SkaffoldConfig) (*Sk | |||
| return nil, errors.Wrap(err, "parsing skaffold deploy config") | |||
| } | |||
|
|
|||
| deployer = label.WithLabels(deployer, labels.Merge(opts, builder, deployer, tagger)) | |||
There was a problem hiding this comment.
This will greedily evaluate the labels for each component, which means any labels added during the run won't get added (e.g. profiles when we add labels for those). Can we call labels.Merge() right before we actually apply the labels?
There was a problem hiding this comment.
Makes sense. Let me fix that
|
@nkubala I've fixed the greedy evaluation. |
dgageot
force-pushed
the
labels
branch
4 times, most recently
from
39c2d88
to
1e5bd33
Compare
|
cc @nkubala to take a look |
Signed-off-by: David Gageot <[email protected]>
No description provided.