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 1) Refactor schema "latest" to "latest/v1" #5728

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Apr 27, 2021

Related: #5727

Description

  • In pkg/skaffold/schema, move ./latest/* to ./latest/v1/* (so as later on once we add v2, v1 and v2 can both be imported as latest)
  • Update the reference of "latest" to "latest_v1" (pkg/ cmd/ integration/)
  • Fix build.ArtifactDependencies reference in pkg/build/maven.

Note: We use alias latest_v1 instead of v1 to import package pkg/skaffold/schema/latest/v1.
The "latest" prefix is to distinguish another "v1" package which is also under pkg/skaffold/schema

Follow-up Work

  • add v3alpha*
  • create latest/v2 to track v3 API schema
  • bisect the latest_v1 reference to accept v2 packages.

To reviewers

the main change is about the new directory pkg/skaffold/schema/latest/v1, whose config.go and upgrade.go are from pkg/skaffold/schema/latest. The other 240+ files are just updating the package import to "latest/v1".

@yuwenma
Copy link
Contributor Author

yuwenma commented Apr 27, 2021

@loosebazooka @chanseokoh Please skip reviewing the PR (somehow I can't remove the auto-selected reviewers). we have some previous discussions about this refactoring. @tejal29 and @marlon-gamez have more context.

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #5728 (bfccb7c) into master (e8f3c65) will decrease coverage by 0.03%.
The diff coverage is 80.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5728      +/-   ##
==========================================
- Coverage   70.88%   70.85%   -0.04%     
==========================================
  Files         421      421              
  Lines       16087    16087              
==========================================
- Hits        11404    11399       -5     
- Misses       3849     3851       +2     
- Partials      834      837       +3     
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/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 46.42% <0.00%> (ø)
cmd/skaffold/app/cmd/test.go 46.66% <0.00%> (ø)
cmd/skaffold/app/cmd/util.go 77.77% <ø> (ø)
pkg/skaffold/build/bazel/build.go 67.24% <ø> (ø)
... and 128 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 68d2918...bfccb7c. Read the comment docs.

@yuwenma yuwenma force-pushed the schema-refactor-1 branch from 1b08ed5 to 3a0e3ec Compare April 27, 2021 17:13
hack/versions/pkg/schema/version.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

I assume the main point of this change is so that we can use latest_v1 and latest_v2 alongside each other right? could you maybe give a hypothetical example of how this might look just to help clarify the motivation for this?

also, would we want a plan for formally deprecating the old config style? or will we want to keep it around indefinitely?

@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Apr 27, 2021
@container-tools-bot
Copy link

Please visit http://35.236.9.12:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 27, 2021
@yuwenma
Copy link
Contributor Author

yuwenma commented Apr 28, 2021

I assume the main point of this change is so that we can use latest_v1 and latest_v2 alongside each other right? could you maybe give a hypothetical example of how this might look just to help clarify the motivation for this?

@nkubala Yeah, you can find the motivations, examples and the year-long deprecation in this code-refactoring doc "Versioning the latest package" section (attached in issue #5727)

@yuwenma yuwenma force-pushed the schema-refactor-1 branch from 1e82bc3 to ca8cafb Compare April 28, 2021 02:51
@yuwenma
Copy link
Contributor Author

yuwenma commented Apr 28, 2021

Rebased to head and fixed merging conflict.

@yuwenma yuwenma requested a review from MarlonGamez April 28, 2021 02:53
@yuwenma yuwenma force-pushed the schema-refactor-1 branch 2 times, most recently from a3ac4e8 to ce15b78 Compare April 28, 2021 02:57
See details in [doc](https://docs.google.com/document/d/1GFy_5ya-ajjSVe1t6f7IWUlAQu35eTHAZZ1GkJBiqcA/edit?resourcekey=0-sKE-yXjuymQ0w2qqNXnTzw#heading=h.az97ji6gorca)

- In pkg/skaffold/schema, move ./latest/* to ./latest/v1/* (so as later on once we add v2, v1 and v2 can both be imported as latest)
- Update the reference of "latest" to "latest_v1" (pkg/ cmd/ integration/)
- Fix build.ArtifactDependencies reference in pkg/build/maven.

Note: We use alias `latest_v1` instead of `v1` to import package pkg/skaffold/schema/latest/v1.
The "latest" prefix is to distinguish another "v1" package which is also under pkg/skaffold/schema

Next: some latest_v1 reference may be bisected for v2 package.
@yuwenma yuwenma force-pushed the schema-refactor-1 branch from ce15b78 to bfccb7c Compare April 28, 2021 03:01
@yuwenma
Copy link
Contributor Author

yuwenma commented Apr 28, 2021

The Travis CI failure seems irrelevant to this PR.

@tejal29 Do you have any insights what may go wrong? The message "/Users/tejaldesai/Downloads/google-cloud-sdk2/" (in code) looks suspicious.

Screen Shot 2021-04-27 at 10 28 35 PM

@tejal29 tejal29 dismissed MarlonGamez’s stale review April 28, 2021 17:24

made the changes

@tejal29
Copy link
Contributor

tejal29 commented Apr 28, 2021

The Travis CI failure seems irrelevant to this PR.

@tejal29 Do you have any insights what may go wrong? The message "/Users/tejaldesai/Downloads/google-cloud-sdk2/" (in code) looks suspicious.

Screen Shot 2021-04-27 at 10 28 35 PM

yeah I think @briandealwis saw this too and opened an issue #5733

The tejaldesai/Downloads etc is from test the message. I should get rid of it.

@tejal29
Copy link
Contributor

tejal29 commented Apr 28, 2021

I restarted the test and will merge after

@tejal29 tejal29 merged commit d38ab4d into GoogleContainerTools:master Apr 28, 2021
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.

5 participants