[v3] add design doc for the new render and deploy #5823

yuwenma · 2021-05-12T17:41:06Z

related: #5673

To Reviewers:
you can review the doc from here. The UX still uses the old field names.

codecov · 2021-05-12T17:50:07Z

Codecov Report

Merging #5823 (406616e) into master (36395cc) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5823      +/-   ##
==========================================
- Coverage   70.92%   70.85%   -0.07%     
==========================================
  Files         438      446       +8     
  Lines       16434    16729     +295     
==========================================
+ Hits        11656    11854     +198     
- Misses       3921     3999      +78     
- Partials      857      876      +19

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/inspect_build_env.go	`58.82% <0.00%> (-32.85%)`	⬇️
pkg/skaffold/deploy/deploy_mux.go	`57.89% <0.00%> (-30.11%)`	⬇️
pkg/skaffold/kubernetes/colorpicker.go	`80.00% <0.00%> (-20.00%)`	⬇️
cmd/skaffold/app/cmd/fix.go	`74.41% <0.00%> (-7.64%)`	⬇️
...ffold/kubernetes/portforward/resource_forwarder.go	`80.88% <0.00%> (-6.84%)`	⬇️
pkg/skaffold/debug/debug.go	`32.83% <0.00%> (-5.77%)`	⬇️
pkg/skaffold/kubernetes/logger/log.go	`39.44% <0.00%> (-3.01%)`	⬇️
...affold/kubernetes/portforward/forwarder_manager.go	`50.00% <0.00%> (-2.39%)`	⬇️
pkg/skaffold/deploy/deploy_problems.go	`80.00% <0.00%> (-2.36%)`	⬇️
pkg/skaffold/parser/config.go	`78.47% <0.00%> (-1.09%)`	⬇️
... and 52 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 36395cc...406616e. Read the comment docs.

docs/design_proposals/new-render-and-deploy.md

aaron-prindle · 2021-05-13T07:08:22Z

docs/design_proposals/new-render-and-deploy.md

+   kpt: [string]
+
+   # This field defines the dry manifests from a variety of sources.
+   generate:


Will the default for skaffold fix once this change goes in be to treat "vanilla" deploy yaml files provided as DRY config? For example, once this change is in will skaffold's simple examples/ skaffold.yaml files all have build, render and deploy sections?
Would this yaml be treated as dry config?
https://github.com/GoogleContainerTools/skaffold/blob/master/examples/microservices/leeroy-app/kubernetes/deployment.yaml

What would the skaffold fix yaml look like for:
examples/microservices skaffold.yaml
https://github.com/GoogleContainerTools/skaffold/blob/master/examples/microservices/skaffold.yaml

TLDR; nit: might be helpful to add an example with a no-op render and show how that is handled by skaffold fix with the API update. From the doc I was a bit confused if the file would be added as DRY or WET config in the no-op render examples (I believe it would be DRY and a render: phase would be added but an example might help clarify this)

Yes, I think those yaml files are dry config.
for
https://github.com/GoogleContainerTools/skaffold/blob/master/examples/microservices/skaffold.yaml, it would be like

build: NO CHANGE render: generate: manifests: - leeroy-web/kubernetes - leeroy-app/kubernetes deploy: # The deployer info is tolerated on purpose. portForward: NO CHANGE profiles: NO CHANGE

aaron-prindle · 2021-05-13T07:08:29Z

docs/design_proposals/new-render-and-deploy.md

+
+#### Under the hood
+
+Though the new UI looks similar except moving helm from "deploy" to "render", the actual implementation is very different. Instead of calling the `helm` CLI, the `skaffold-helm-inflator` functions are used to convert the `Helm` charts to a kpt [`ResourceList.items`]（https://googlecontainertools.github.io/kpt/guides/producer/functions/#resourcelistitems). This function is expected to digest all the `helm` arguments listed in the `skaffold.yaml` reference doc [v2beta16](https://skaffold.dev/docs/references/yaml/#deploy-helm).


Are there any features/flags that skaffold+helm supports currently that are not backwards compatible with the skaffold-helm-inflator? If so they might be worth calling out in the migration story.

Good point. The helm-inflator image is not there yet, we will add more doc once that part of work is more stabilized.

aaron-prindle

LGTM, left some nits. Looks great!

aaron-prindle · 2021-05-13T07:11:15Z

Also not sure if you have travis access but I restarted the one job that had a Travis flake on this PR.

aaron-prindle · 2021-05-13T16:41:43Z

docs/design_proposals/new-render-and-deploy.md

+
+### Builtin `kpt`
+
+`skaffold` will have `kpt` embedded via go bindata, which ship together with the skaffold binary to guarantee the versions match.


Are there any performance or binary size concerns w/ this approach? Skaffold is currently ~50Mb and kpt is also ~50Mb. Might make sense to add a blurb here around the possible tradeoffs (if there are any)

Also it might make sense to add a blurb/section around how this will be tested specifically as I don't believe skaffold has integration tests that pull in updated external binary deps atm. Will skaffold pin to a kpt version and be updated ad-hoc? Use the latest kpt release? Perhaps nothing special is needed here (as it is somewhat similar to the situation for other binaries that skaffold calls out to) but kpt seems very tightly coupled in this case.

aaron-prindle · 2021-05-13T17:12:02Z

It might also make sense to add section here about what files/changes will be made the V2 Events proto (from meeting offline) and possibly the process there if it is non-trivial

yuwenma · 2021-05-14T19:54:26Z

It might also make sense to add section here about what files/changes will be made the V2 Events proto (from meeting offline) and possibly the process there if it is non-trivial

I would need to do more research on this. I have a gut feeling that the v2 events proto is more related to our release workflow (currently we release a single API version but in the future there would be two) than the kpt render/deploy part. WDYT?

yuwenma requested a review from a team as a code owner May 12, 2021 17:41

yuwenma requested a review from aaron-prindle May 12, 2021 17:41

google-cla bot added the cla: yes label May 12, 2021

yuwenma requested a review from tejal29 May 12, 2021 17:42

yuwenma force-pushed the design-doc branch from 2f24127 to 6f11b9d Compare May 12, 2021 17:43

aaron-prindle reviewed May 13, 2021

View reviewed changes

docs/design_proposals/new-render-and-deploy.md Outdated Show resolved Hide resolved

aaron-prindle reviewed May 13, 2021

View reviewed changes

docs/design_proposals/new-render-and-deploy.md Outdated Show resolved Hide resolved

aaron-prindle reviewed May 13, 2021

View reviewed changes

aaron-prindle approved these changes May 13, 2021

View reviewed changes

aaron-prindle reviewed May 13, 2021

View reviewed changes

[v3] add design doc for the new render and deploy

406616e

yuwenma force-pushed the design-doc branch from 6f11b9d to 406616e Compare May 14, 2021 19:42

tejal29 merged commit 6523c38 into GoogleContainerTools:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v3] add design doc for the new render and deploy #5823

[v3] add design doc for the new render and deploy #5823

yuwenma commented May 12, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

aaron-prindle May 13, 2021 •

edited

Loading

yuwenma May 14, 2021 •

edited

Loading

aaron-prindle May 13, 2021

yuwenma May 14, 2021

aaron-prindle left a comment

aaron-prindle commented May 13, 2021

aaron-prindle May 13, 2021 •

edited

Loading

aaron-prindle May 13, 2021 •

edited

Loading

aaron-prindle commented May 13, 2021 •

edited

Loading

yuwenma commented May 14, 2021 •

edited

Loading


		#### Under the hood

		Though the new UI looks similar except moving helm from "deploy" to "render", the actual implementation is very different. Instead of calling the `helm` CLI, the `skaffold-helm-inflator` functions are used to convert the `Helm` charts to a kpt [`ResourceList.items`]（https://googlecontainertools.github.io/kpt/guides/producer/functions/#resourcelistitems). This function is expected to digest all the `helm` arguments listed in the `skaffold.yaml` reference doc [v2beta16](https://skaffold.dev/docs/references/yaml/#deploy-helm).


		### Builtin `kpt`

		`skaffold` will have `kpt` embedded via go bindata, which ship together with the skaffold binary to guarantee the versions match.

[v3] add design doc for the new render and deploy #5823

[v3] add design doc for the new render and deploy #5823

Conversation

yuwenma commented May 12, 2021 • edited Loading

codecov bot commented May 12, 2021 • edited Loading

Codecov Report

aaron-prindle May 13, 2021 • edited Loading

Choose a reason for hiding this comment

yuwenma May 14, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-prindle May 13, 2021

Choose a reason for hiding this comment

yuwenma May 14, 2021

Choose a reason for hiding this comment

aaron-prindle left a comment

Choose a reason for hiding this comment

aaron-prindle commented May 13, 2021

aaron-prindle May 13, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-prindle May 13, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-prindle commented May 13, 2021 • edited Loading

yuwenma commented May 14, 2021 • edited Loading

yuwenma commented May 12, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

aaron-prindle May 13, 2021 •

edited

Loading

yuwenma May 14, 2021 •

edited

Loading

aaron-prindle May 13, 2021 •

edited

Loading

aaron-prindle May 13, 2021 •

edited

Loading

aaron-prindle commented May 13, 2021 •

edited

Loading

yuwenma commented May 14, 2021 •

edited

Loading