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

feat: Enable render in skaffold run v2 #6760

Closed
wants to merge 27 commits into from

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Oct 24, 2021

Related: #5673

Description

Since render is necessary in the new skaffold pipeline. skaffold run should have render enabled.

yuwenma and others added 27 commits July 7, 2021 07:54
latest/v2 used to depend on latest/v1 for build, tag, test. This is aimed at making sure the v1 changes on these steps
can be backportting to v2. This requires engineers to update the v2 reference (e.g. bumps the latestV1 reference to a newer release)
and sometimes change the v2 pipeline as well. Since v2 is not released yet, this work are not officialized and v2 always fells behind
the v1 changes.

This change makes latest/v2 no longe relies on v1 schema.
* [v2] Switch to v2

1. Remove V1 runner to minimize the CLI binary size.
2. Update the mainstream schemas reference from v1 to v2.
3. Fix unittests.
4. Cherry-pick e465820 "Add node selector option..."

* [v2] integration test and env

1. update integration test schema to v3alpha2.
2. update check-samples to check against latest v2.
3. skip the integration test that does not have v2 support yet. (shall be re-enabled once the corresponding cmd is added).
* [v2] Add a new deployer kptV2.

* [v2] Add proto errors for kpt deployer v2.
…ContainerTools#6270)

* [backport] Add original dev, load_images and deploy logics to runner.v2

* [v2] Enable `skaffold dev`

1. Add kptV2 deployer to the deployerMux initialization.
2. Add `hydrationDir` as a RunContext field, which is used for both Renderer and Deployer to cache and hydrate the manifests.
   - default to <WORKDIR>/.kpt-pipeline
   - can be overriden by `--output` flag in `skaffold render` (Render Step)
   - can be overriden by `.deploy.kpt.dir` field in skaffold.yaml in `skaffold dev` and `skaffold run` (Deploy step)
3. Render is used to be called by deployers in `skaffold dev` and `skaffold run`. This logic is now removed.

Follow-up work:
1. Update kubectl deployer (default) to apply the manifests from hydrationDir.
2. Dev re-render
3. Enable other commands

* [v2] enable the "helm" in "manifest.generate"
…Tools#6339)

Root cause: The render manfiests are registered to the filewatcher before "Render" func is called. However,
the renderer has its dependencies calculated and refreshed in each "Render" call.

This PR separates the "Render" and "manfiestDependencies" completely.
…ools#6346)

* [v2] Customize hydration-dir and render-output path.

1. Remove unused flag v2
2. Enable hydration-dir as a configurable flag.
3. Convert structured (hydrated) manifests to a flattened format and stored in a give path (--render-output/--output flag)
4. Prompt user interactive message to acknowledge users that the manifest hydration may override the hydration-dir
5. sort SkaffoldOptions to fix maligned linter error.

* Skaffold master -> main (GoogleContainerTools#6263)

- Rewrite the few existing uses of `skaffold/(blob|tree)/HEAD` to use `main`
  as GitHub's file viewer uses a commit rather than a branch.
- Rewrite the buildpacks references to use `main` since they already switched.
- Explicitly specify `main` for remote repositories due to GoogleContainerTools#6264

* bump v1 versions to v2beta20

* improve user experience on the prompting message

Co-authored-by: Brian de Alwis <[email protected]>
This backports main branch's apply.
…Hook (GoogleContainerTools#6388)

Basic backport changes. PR has been waited for a while.
Since render is necessary in the new skaffold pipeline. `skaffold run` should have render enabled.
@yuwenma yuwenma requested a review from nkubala October 24, 2021 01:16
@yuwenma yuwenma requested a review from a team as a code owner October 24, 2021 01:16
@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #6760 (434fd7c) into main (823896b) will increase coverage by 0.28%.
The diff coverage is 65.21%.

❗ Current head 434fd7c differs from pull request most recent head ac48c6d. Consider uploading reports for the commit ac48c6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6760      +/-   ##
==========================================
+ Coverage   69.94%   70.23%   +0.28%     
==========================================
  Files         478      477       -1     
  Lines       18286    21820    +3534     
==========================================
+ Hits        12791    15326    +2535     
- Misses       4548     5485     +937     
- Partials      947     1009      +62     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/apply.go 25.00% <0.00%> (ø)
cmd/skaffold/app/cmd/delete.go 55.55% <0.00%> (-1.59%) ⬇️
cmd/skaffold/app/cmd/deploy.go 53.84% <0.00%> (-2.68%) ⬇️
cmd/skaffold/app/cmd/filter.go 25.00% <0.00%> (-0.59%) ⬇️
cmd/skaffold/app/cmd/find_configs.go 48.64% <0.00%> (+0.16%) ⬆️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <0.00%> (-1.54%) ⬇️
cmd/skaffold/app/cmd/render.go 41.37% <0.00%> (+0.63%) ⬆️
cmd/skaffold/app/cmd/test.go 44.44% <0.00%> (-2.23%) ⬇️
pkg/skaffold/build/build.go 0.00% <ø> (ø)
pkg/skaffold/build/cache/cache.go 53.57% <ø> (+1.57%) ⬆️
... and 541 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 823896b...ac48c6d. Read the comment docs.

@google-cla
Copy link

google-cla bot commented Oct 24, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 24, 2021
@yuwenma yuwenma closed this Oct 24, 2021
@yuwenma
Copy link
Contributor Author

yuwenma commented Oct 24, 2021

wrong base branch. This is v2 feature and cannot be merged to master until v2 is merged.

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.

2 participants