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

[v3] (part 2) Move the v1 runner components to pkg/skaffold/runner/v1. #5781

Merged
merged 1 commit into from
May 6, 2021

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented May 4, 2021

Fixes: #nnn
Related: #5727
Merge after: #5780

Description
Changes include:

  • imports changes
  • public/private functions and attributes
  • Simplified the runner.Tester

@yuwenma yuwenma requested a review from a team as a code owner May 4, 2021 09:03
@yuwenma yuwenma requested a review from briandealwis May 4, 2021 09:03
@google-cla google-cla bot added the cla: yes label May 4, 2021
@yuwenma yuwenma requested review from tejal29 and removed request for briandealwis May 4, 2021 09:03
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #5781 (ebcbdd9) into master (ce34201) will decrease coverage by 0.02%.
The diff coverage is 85.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5781      +/-   ##
==========================================
- Coverage   70.95%   70.93%   -0.03%     
==========================================
  Files         439      438       -1     
  Lines       16408    16410       +2     
==========================================
- Hits        11642    11640       -2     
- Misses       3913     3916       +3     
- Partials      853      854       +1     
Impacted Files Coverage Δ
pkg/skaffold/runner/builder.go 72.72% <ø> (ø)
pkg/skaffold/runner/v1/apply.go 0.00% <ø> (ø)
pkg/skaffold/runner/v1/cleanup.go 0.00% <ø> (ø)
pkg/skaffold/runner/v1/debugging.go 66.66% <ø> (ø)
pkg/skaffold/runner/v1/generate_pipeline.go 0.00% <ø> (ø)
pkg/skaffold/runner/v1/load_images.go 94.73% <ø> (ø)
pkg/skaffold/runner/v1/logger.go 40.00% <ø> (ø)
pkg/skaffold/runner/v1/portforwarder.go 22.22% <ø> (ø)
pkg/skaffold/runner/v1/render.go 12.50% <0.00%> (ø)
pkg/skaffold/runner/v1/runner.go 0.00% <0.00%> (ø)
... and 12 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 ce34201...ebcbdd9. Read the comment docs.

@yuwenma yuwenma force-pushed the runner-2-v1-dir branch from c428ebf to 4796707 Compare May 4, 2021 11:55
@yuwenma yuwenma added this to the v1.24.0 milestone May 4, 2021
@yuwenma yuwenma added the area/v3 V3 label May 4, 2021
@yuwenma yuwenma force-pushed the runner-2-v1-dir branch 2 times, most recently from 05a7bc3 to f008c5d Compare May 5, 2021 22:06
@yuwenma
Copy link
Contributor Author

yuwenma commented May 5, 2021

Rebased after #5780 is merged.

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.

Few questions:

  • Should pkg/skaffold/runner/build_deploy.go be moved to pkg/skaffold/runner/v1/build_deploy.go?
  • Should pkg/skaffold/runner/v1/build_deploy_test.go be deleted?
  • Should pkg/skaffold/runner/prune.go be moved as well?

cmd/skaffold/app/cmd/runner.go Show resolved Hide resolved
@yuwenma yuwenma force-pushed the runner-2-v1-dir branch 3 times, most recently from 971ce77 to 823575c Compare May 5, 2021 22:58
@yuwenma
Copy link
Contributor Author

yuwenma commented May 6, 2021

Few questions:

  • Should pkg/skaffold/runner/build_deploy.go be moved to pkg/skaffold/runner/v1/build_deploy.go?

Both v1 and v2 will share the build_deploy logic, so it does not need to be moved (It calls a Deploy (in Deploy.go) function which will be different between v1 and v2).

  • Should pkg/skaffold/runner/v1/build_deploy_test.go be deleted?

Conflict resolved. pkg/skaffold/runner/v1/build_deploy_test.go is moved to runner/v1 to make sure the v1 build_deploy works. another build_deploy_test.go will be added to v2 for the v2 build_deploy functionality.

  • Should pkg/skaffold/runner/prune.go be moved as well?

prune.go is still needed.

@yuwenma yuwenma force-pushed the runner-2-v1-dir branch from 823575c to ebcbdd9 Compare May 6, 2021 00:58
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.

Dicussed offline

  1. We want to go to a structure
runner/
  - base/
  - v1/
  - v2/

To do that, @tejal29 and @yuwenma will explore the using

  1. util.VersionedConfig
  2. Add interfaces for Artifact and Pipeline and get rid of latest_v1.Artifact and latest_v1.Pipeline

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