Add forwarding deployer muxer to enable multiple deployers by corneliusweig · Pull Request #3391 · GoogleContainerTools/skaffold

corneliusweig · 2019-12-16T00:22:57Z

Should merge before : #3392

Description

Add a deployer implementation (DeployerMux) that multiplexes the deployer actions with multiple deployer instances. This new deployer is simply a []deploy.Deployer which forwards the commands of the Deployer interface to the instances in the list. When encountering an error, DeployerMux aborts and returns this error. Otherwise it collects the outputs and returns all results in bulk.

This is a pre-requisite for enabling multiple deployers in skaffold.yaml.

User facing changes

n/a

This PR only adds the functionality but does not enable anything.

Next PRs.

adapt skaffold.yaml to accept multiple deployer configurations in the deploy field.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes unit tests
Mentions any output changes.
Adds documentation as needed: user docs, YAML reference, CLI reference.
Adds integration tests if needed.

Reviewer Notes

The code flow looks good.
Unit test added.
User facing changes look good.

codecov · 2019-12-16T00:30:38Z

Codecov Report

Merging #3391 into master will increase coverage by 0.12%.
The diff coverage is 87.71%.

Impacted Files	Coverage Δ
pkg/skaffold/deploy/kubectl.go	`65.87% <100%> (+2.16%)`	⬆️
pkg/skaffold/deploy/util.go	`79.16% <80%> (+0.21%)`	⬆️
pkg/skaffold/deploy/deploy_mux.go	`89.13% <89.13%> (ø)`
...affold/kubernetes/portforward/kubectl_forwarder.go	`65.85% <0%> (-2.44%)`	⬇️
pkg/skaffold/deploy/labels.go	`13.41% <0%> (+3.65%)`	⬆️

balopat · 2019-12-18T04:23:14Z

pkg/skaffold/deploy/deploy_mux.go

+// it collects the results and returns it in bulk.
+type DeployerMux []Deployer
+
+var _ Deployer = DeployerMux{}


What does this line do?

gotcha, it is just a compile time check to make sure that DeployerMux implements the Deployer interface.

a comment would be nice?

Exactly, this was my local compile check that I have implemented the interface. As this pattern seems to be unused in Skaffold, I opted to remove the line instead.

balopat

This looks good, but I have some small comments on set usage and the labelling strategy.

balopat · 2019-12-18T04:51:30Z

pkg/skaffold/deploy/deploy_mux.go

+func (m DeployerMux) Labels() map[string]string {
+	labels := make(map[string]string)
+	for _, deployer := range m {
+		copyMap(labels, deployer.Labels())


I don't think this is what we want, if I understand this correctly this will result in a single label with skaffold.dev/deployer: <last deployer>.

The best would be to label resources with their own deployers. However that seems hard with this current architecture.

So I'd rather have something like:

skaffold.dev/deployer0: helm skaffold.dev/deployer1: kubectl skaffold.dev/deployer2: kustomize

WDYT?

Good catch. This was already addressed with #3390, where the deployer labels are removed from the global list of labellers and are applied just in the specific deployer instead.
So I think the semantics of this line is correct.

balopat · 2019-12-18T05:00:28Z

pkg/skaffold/deploy/deploy_mux.go

+}
+
+func (m DeployerMux) Deploy(ctx context.Context, w io.Writer, as []build.Artifact, ls []Labeller) *Result {
+	seenNamespaces := sets.String{}


this is cool, but kinda scares me that it's in kubernetes. Haven't made up my mind yet about it.
The risk of it disappearing / changing is minimal as skaffold is for kubernetes. But in general we tend to avoid depending on utilities from dependencies - unless the dependency is specifically for that utility. E.g. docker has some great stuff in their ioutils package but we decided not to use them.

Yeah that totally makes sense. The de-duplication is easily implemented so there's no need to add a dependency here.
I think there are a few places where Skaffold code could benefit from a StringSet util, but this is not the right PR to make that change :). So I've inlined a small helper in this file.

pkg/skaffold/deploy/deploy_mux.go

This allows re-use by other deployer renderers. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

This Deployer implemenation forwards commands to all of the deployers in his list. It will be useful to allow multiple deployers in skaffold.yaml. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

corneliusweig · 2020-01-04T13:29:09Z

Sorry for my confusion. I accidentally committed the review comments to the wrong branch 2 weeks ago 🤦‍♂️
Please have another look :)

dgageot

LGTM with a nit

pkg/skaffold/deploy/deploy_mux.go

Changes were made. Looks good

corneliusweig requested review from balopat, dgageot, nkubala and tejal29 as code owners December 16, 2019 00:22

pull-request-size bot added the size/L label Dec 16, 2019

googlebot added the cla: yes label Dec 16, 2019

corneliusweig mentioned this pull request Dec 16, 2019

Enable multiple deployers in skaffold.yaml #3392

Merged

7 tasks

balopat added the kokoro:run runs the kokoro jobs on a PR label Dec 18, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Dec 18, 2019

balopat reviewed Dec 18, 2019

View reviewed changes

balopat previously requested changes Dec 18, 2019

View reviewed changes

balopat added the pr/changes-requested label Dec 18, 2019

corneliusweig added 3 commits January 4, 2020 13:41

Extract writing of rendered manifests

0b920e6

This allows re-use by other deployer renderers. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Add delegating Deployer muxer

5a06098

This Deployer implemenation forwards commands to all of the deployers in his list. It will be useful to allow multiple deployers in skaffold.yaml. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Review comments: prefer not to depend on external utils

be44da2

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

corneliusweig force-pushed the w/deployer-mux/muxer branch from cc58e63 to be44da2 Compare January 4, 2020 12:45

dgageot self-assigned this Jan 15, 2020

dgageot removed the pr/changes-requested label Jan 15, 2020

dgageot approved these changes Jan 15, 2020

View reviewed changes

pkg/skaffold/deploy/deploy_mux.go Show resolved Hide resolved

dgageot approved these changes Jan 15, 2020

View reviewed changes

dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 15, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 15, 2020

dgageot merged commit f2aeb3a into GoogleContainerTools:master Jan 15, 2020

corneliusweig deleted the w/deployer-mux/muxer branch January 15, 2020 20:39

Conversation

corneliusweig commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balopat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corneliusweig commented Jan 4, 2020

Uh oh!

dgageot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

corneliusweig commented Dec 16, 2019 •

edited

Loading

codecov bot commented Dec 16, 2019 •

edited

Loading

balopat left a comment •

edited

Loading