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

Use default deployer in 'skaffold apply' #5776

Merged
merged 2 commits into from
May 10, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented May 3, 2021

EDIT: merge after: #5791

#5543 added support for skaffold apply to send rendered manifests to a cluster, but didn't plumb through the creation of the default deployer all the way. this change makes sure that always happen with the apply command is issued.

@nkubala nkubala requested a review from a team as a code owner May 3, 2021 23:30
@nkubala nkubala requested a review from aaron-prindle May 3, 2021 23:30
@google-cla google-cla bot added the cla: yes label May 3, 2021
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #5776 (9dc311e) into master (8efc9ef) will decrease coverage by 0.00%.
The diff coverage is 82.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5776      +/-   ##
==========================================
- Coverage   71.00%   71.00%   -0.01%     
==========================================
  Files         438      439       +1     
  Lines       16497    16495       -2     
==========================================
- Hits        11714    11712       -2     
  Misses       3921     3921              
  Partials      862      862              
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/apply.go 25.00% <0.00%> (ø)
cmd/skaffold/app/cmd/delete.go 57.14% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 56.52% <0.00%> (ø)
cmd/skaffold/app/cmd/diagnose.go 64.51% <ø> (ø)
cmd/skaffold/app/cmd/filter.go 25.58% <0.00%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.48% <0.00%> (ø)
cmd/skaffold/app/cmd/generate_pipeline.go 61.53% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 40.74% <0.00%> (ø)
cmd/skaffold/app/cmd/runner.go 58.49% <0.00%> (ø)
cmd/skaffold/app/cmd/test.go 46.66% <0.00%> (ø)
... and 131 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb9c352...9dc311e. Read the comment docs.

examples/helm-deployment/render.yaml Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@ func TestGetDeployer(tOuter *testing.T) {
cfg latest_v1.DeployType
helmVersion string
expected deploy.Deployer
apply bool
Copy link
Member

Choose a reason for hiding this comment

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

So we're using apply = true to implicitly test getDefaultDeployer(). I think it would be better to test getDefaultDeployer() explicitly, and add some more cases like Kustomize flags and conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is actually another set of test cases below this one that tests getDefaultDeployer() explicitly - the main thing I changed here is to embed the if runCtx.Opts.Apply check in the call to getDeployer() which sends us through to getDefaultDeployer(), which I thought cleaned up the code in NewForConfig() a bit.

the addition of the apply boolean to this set of tests just tests the branching logic that ensures if we're in skaffold apply, we get a default deployer - the other tests make sure that all the other cases (e.g. Kustomize flags, unresolvable config, etc.) are working as intended.

@nkubala
Copy link
Contributor Author

nkubala commented May 5, 2021

CI is failing because of an issue with skaffold diagnose, which is fixed by #5791

@nkubala nkubala force-pushed the apply-default-deployer branch 3 times, most recently from 96a3cbb to 3c17376 Compare May 6, 2021 22:07
@nkubala nkubala force-pushed the apply-default-deployer branch from 3c17376 to 9dc311e Compare May 10, 2021 16:48
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

lgtm 👍🏼

@nkubala nkubala merged commit 73e7748 into GoogleContainerTools:master May 10, 2021
@nkubala nkubala deleted the apply-default-deployer branch May 10, 2021 23:06
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.

3 participants