Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable multiple deployers in skaffold.yaml #3392

Merged

Conversation

corneliusweig
Copy link
Contributor

@corneliusweig corneliusweig commented Dec 16, 2019

Relates to #2875, #3292

Fixes #2875, #3292

Should merge before : n/a

Should merge after : #3391, #3390

Description

Some users find it limiting that they can only configure a single deployer in Skaffold (#2875, #3292). This PR enables using multiple deployers.

User facing changes

Users get the possibility to configure multiple deployers.

⚠️ When merging profiles into the base pipeline, Skaffold used to wipe other deployers. This is no longer the case, because Skaffold now handles more than one deployer.

Before

skaffold.yaml allows to configure exactly one deployer (kubectl, kustomize, helm).

After

Users can configure all three deployers at once. Iow, the oneOf=deployer constraint in skaffold.yaml is dropped.

Next PRs.

n/a

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.

Release Notes

- The deployer section in `skaffold.yaml` now accepts multiple deployers in a single pipeline.
- When applying profiles, deployers in the base profile no longer get wiped when merging in the deployer from the profile.

@corneliusweig
Copy link
Contributor Author

I went with the approach to simply drop the oneOf constraint. Another option would have been to add a list of deployers in the deploy section, like so:

deploy:
  kubeContext: something
  deployers:
  - kustomize: {}
  - kubectl: {}
  - helm: {}
  - ...

However, the nested deploy.deployers looks very weird and it is not clear if a list of deployers would even be desirable.

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #3392 into master will increase coverage by 0.55%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/build/cluster/kaniko.go 0% <0%> (-55.13%) ⬇️
pkg/skaffold/deploy/kustomize.go 73.75% <0%> (-2.86%) ⬇️
pkg/skaffold/generate_pipeline/profile.go 32.87% <0%> (-2.65%) ⬇️
pkg/skaffold/build/jib/jib.go 72.79% <0%> (-0.7%) ⬇️
pkg/skaffold/deploy/kubectl/visitor.go 93.54% <0%> (-0.21%) ⬇️
pkg/skaffold/build/jib/maven.go 100% <0%> (ø) ⬆️
pkg/skaffold/build/build.go 0% <0%> (ø) ⬆️
pkg/skaffold/docker/syncmap.go 67.16% <0%> (ø) ⬆️
pkg/skaffold/build/jib/gradle.go 100% <0%> (ø) ⬆️
pkg/skaffold/build/gcb/buildpacks.go 100% <0%> (ø) ⬆️
... and 27 more

@corneliusweig corneliusweig changed the title Enable multiple deployers in skaffold.yaml WIP Enable multiple deployers in skaffold.yaml Dec 16, 2019
This allows re-use by other deployer renderers.

Signed-off-by: Cornelius Weig <[email protected]>
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 <[email protected]>
Some use-cases require a hybrid mode where users need to
use different deployers at the same time.

Signed-off-by: Cornelius Weig <[email protected]>
So far, deployers were marked with `oneOf`, so that only one of them
could be active at a time. Since this no longer holds, profile
activation now merges the deployers from profiles and base.
This change adapts the tests accordingly.

Signed-off-by: Cornelius Weig <[email protected]>
This is necessary because the new logic merges profile deployer
into the main pipeline instead of replacing existing deployers.

Signed-off-by: Cornelius Weig <[email protected]>
@corneliusweig corneliusweig force-pushed the w/deployer-mux/config branch from d746cfc to ec3eaf0 Compare January 4, 2020 12:56
@corneliusweig corneliusweig changed the title WIP Enable multiple deployers in skaffold.yaml Enable multiple deployers in skaffold.yaml Jan 4, 2020
@corneliusweig
Copy link
Contributor Author

I've finally added the missing integration test, so it's no longer WIP :)

@balopat balopat self-assigned this Jan 15, 2020
@balopat balopat removed the !! wip !! label Jan 15, 2020
@@ -23,7 +23,7 @@ Skaffold supports the following tools for deploying applications:
The `deploy` section in the Skaffold configuration file, `skaffold.yaml`,
controls how Skaffold builds artifacts. To use a specific tool for deploying
artifacts, add the value representing the tool and options for using the tool
to the `deploy` section.
to the `deploy` section. You may even use different tools at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to the `deploy` section. You may even use different tools at the same time.
to the `deploy` section. You may even use a combination of each available deployment approach at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll not apply this suggestion right now, to not interfere with the kokoro run. But please go ahead and apply this if you think it's ready for merging!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed it - will merge as soon as kokoro is done again :)

@balopat balopat added kokoro:run runs the kokoro jobs on a PR and removed needs-rebase labels Jan 15, 2020
@balopat
Copy link
Contributor

balopat commented Jan 15, 2020

I just rebased it - hope that's fine!

@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 15, 2020
@corneliusweig
Copy link
Contributor Author

I just rebased it - hope that's fine!

Yes of course!

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Jan 16, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 16, 2020
@dgageot dgageot merged commit fd09f86 into GoogleContainerTools:master Jan 16, 2020
@corneliusweig corneliusweig deleted the w/deployer-mux/config branch January 16, 2020 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use 2 or more deploy tools in 1 skaffold file
5 participants