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 skaffold debug for kustomize #2043

Merged
merged 9 commits into from
May 15, 2019

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Apr 30, 2019

The list of manifest transforms is within the pkg/skaffold/deploy package, so the kustomize deployer just applies that same list before passing through to kubectl.

Required adding support for passing through insecureRegistries.

Easy verification test:

$ examples/kustomize
$ skaffold debug -v debug
...
INFO[0000] Image [index.docker.io/library/busybox] not configured for debugging: no build artifact for ["index.docker.io/library/busybox"] 
...

That message is from the debugging transforms (so they're being used). The debug transforms can't be applied as the project references a prebuilt image and doesn't actually have a build artifact.

Apply manifest transformations before passing through to kubectl.
Required adding support for passing through insecureRegistries.
@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #2043 into master will increase coverage by 0.09%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2043      +/-   ##
==========================================
+ Coverage   56.18%   56.27%   +0.09%     
==========================================
  Files         180      180              
  Lines        7771     7799      +28     
==========================================
+ Hits         4366     4389      +23     
- Misses       2988     2992       +4     
- Partials      417      418       +1
Impacted Files Coverage Δ
pkg/skaffold/deploy/kubectl.go 69.33% <0%> (ø) ⬆️
pkg/skaffold/deploy/kustomize.go 73.91% <33.33%> (-3.1%) ⬇️
pkg/skaffold/build/custom/dependencies.go 75% <0%> (-10.72%) ⬇️
pkg/skaffold/sync/sync.go 70.32% <0%> (-4.06%) ⬇️
pkg/skaffold/util/tar.go 35.71% <0%> (-0.44%) ⬇️
pkg/skaffold/runner/labeller.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/validation/validation.go 96.87% <0%> (+0.57%) ⬆️
pkg/skaffold/schema/v1beta9/upgrade.go 84.9% <0%> (+18.23%) ⬆️

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 a117236...1880101. Read the comment docs.

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

We should add some intergration test like in https://github.com/GoogleContainerTools/skaffold/blob/master/integration/deploy_test.go#L78 where we run skaffold debug for jib examples and see if the deployment "web" has the applied transformation.

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no and removed cla: yes labels May 8, 2019
@briandealwis
Copy link
Member Author

briandealwis commented May 8, 2019

PTAL @tejal29. This adds an integration test that runs skaffold debug in integration/testdata/jib and verified that the affected deployment has an annotation.

There's a seemingly unrelated change to RunBuilder.RunOrFailOutput() to use cmd.CombinedOutput() instead of cmd.Output() to include stderr. I originally modelled the test on run_test.go which uses RunOrFailOutput(), but was failing but the error output wasn't being shown:

$ go test -v -run TestDebugKubectl
=== RUN   TestDebugKubectl
=== RUN   TestDebugKubectl/jib
INFO[0000] Namespace: skaffoldbn6fk                     
INFO[0000] [skaffold debug --namespace skaffoldbn6fk]   
--- FAIL: TestDebugKubectl (0.36s)
    --- FAIL: TestDebugKubectl/jib (0.36s)
        debug_test.go:52: skaffold debug: exit status 1, 
FAIL
exit status 1
FAIL	github.com/GoogleContainerTools/skaffold/integration	0.393s

With this fix, I then get the actual error text:

 --- FAIL: TestDebugKubectl/jib (0.54s)
        debug_test.go:52: skaffold debug: exit status 1, time="2019-05-08T15:06:52-04:00" level=fatal msg="creating runner: parsing skaffold config: unknown api version: 'skaffold/v1beta10'"

(These tests seem to use skaffold from my PATH?)

UPDATE: backed out this change as it causes failures in fix_test.go.

@googlebot

This comment has been minimized.

@briandealwis
Copy link
Member Author

Oops, hold off: I didn't add a kustomize test.

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@briandealwis briandealwis requested a review from tejal29 May 8, 2019 20:35
@briandealwis
Copy link
Member Author

briandealwis commented May 8, 2019

Unrelated failure on Kokoro, I think:

=== RUN   TestFix
time="2019-05-08T20:23:05Z" level=info msg="Namespace: skaffoldr2zqk"
time="2019-05-08T20:23:05Z" level=info msg="[skaffold fix -f skaffold.yaml]"
time="2019-05-08T20:23:05Z" level=info msg="Ran in 32.346366ms"
time="2019-05-08T20:23:05Z" level=info msg="[skaffold run --namespace skaffoldr2zqk -f -]"
time="2019-05-08T20:23:05Z" level=warning msg="Your Skaffold version might be too old. Download the latest version (0.28.0) at https://storage.googleapis.com/skaffold/releases/latest/skaffold-linux-amd64\n"
time="2019-05-08T20:23:05Z" level=fatal msg="creating runner: parsing skaffold config: parsing api version: yaml: line 3: could not find expected ':'"
--- FAIL: TestFix (0.37s)
    fix_test.go:35: skaffold run: exit status 1

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no and removed cla: yes labels May 9, 2019
balopat
balopat previously requested changes May 9, 2019
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

The kokoro TestFix error is not unrelated - the test is checking the output of the command that you changed by switching to CombinedOutput.

@briandealwis
Copy link
Member Author

Ah! Thank you! 🙏

@briandealwis
Copy link
Member Author

Backed out the change to use CombinedOutput since it's not required.

@googlebot

This comment has been minimized.

@briandealwis briandealwis requested a review from balopat May 10, 2019 01:44
@briandealwis
Copy link
Member Author

PTAL: tests passing

@tejal29 tejal29 dismissed balopat’s stale review May 15, 2019 21:16

fixed combinedOutput

@tejal29 tejal29 merged commit db380bf into GoogleContainerTools:master May 15, 2019
@briandealwis briandealwis deleted the kstmzdbg branch November 21, 2019 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants