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

Remove plugin code from config, upgrade to v1beta10 #2016

Merged
merged 5 commits into from
Apr 25, 2019

Conversation

priyawadhwa
Copy link
Contributor

Continues work on #1988

@codecov-io
Copy link

codecov-io commented Apr 24, 2019

Codecov Report

Merging #2016 into master will increase coverage by 0.34%.
The diff coverage is 65.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2016      +/-   ##
==========================================
+ Coverage   55.49%   55.84%   +0.34%     
==========================================
  Files         173      175       +2     
  Lines        7512     7498      -14     
==========================================
+ Hits         4169     4187      +18     
+ Misses       2952     2913      -39     
- Partials      391      398       +7
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/run.go 37.5% <ø> (+1.5%) ⬆️
pkg/skaffold/build/gcb/desc.go 94.28% <ø> (+26.93%) ⬆️
pkg/skaffold/runner/context/context.go 61.29% <ø> (+2.95%) ⬆️
pkg/skaffold/schema/v1beta8/config.go 100% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/dev.go 21.56% <ø> (+0.81%) ⬆️
pkg/skaffold/schema/v1beta8/upgrade.go 66.66% <ø> (ø) ⬆️
pkg/skaffold/schema/defaults/defaults.go 89.85% <ø> (-1.18%) ⬇️
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/apiversion/apiversion.go 100% <ø> (ø) ⬆️
pkg/skaffold/schema/versions.go 74.35% <ø> (ø) ⬆️
... and 11 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 f28b472...a094417. Read the comment docs.

}

// convert Build (should be same)
var newBuild next.BuildConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Notes, you mention "Removed all schemas associated with the builder plugin".
Right now, looks like you are ignoring the plugin (looking at the upgrade_test.go) which will work for "docker" plugin since docker Artifact is the default.
If the plugin type was something else, like "bazel", we need to convert it to old style where artifact type will be now "bazel"?
Basically, https://github.com/GoogleContainerTools/skaffold/blob/v0.27.0/pkg/skaffold/build/plugin/plugin.go#L51
flow where bazel can appear https://github.com/GoogleContainerTools/skaffold/blob/v0.27.0/pkg/skaffold/build/plugin/core.go#L36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, added a fix for that in upgrade and a test in upgrade_test

@priyawadhwa priyawadhwa requested a review from tejal29 April 25, 2019 00:10
@priyawadhwa priyawadhwa merged commit 79d7be9 into GoogleContainerTools:master Apr 25, 2019
@priyawadhwa priyawadhwa deleted the remove branch April 25, 2019 21:45
@priyawadhwa priyawadhwa mentioned this pull request Apr 25, 2019
priyawadhwa pushed a commit to priyawadhwa/skaffold-1 that referenced this pull request Apr 25, 2019
I accidentally upgraded to v1beta10 in GoogleContainerTools#2016 when v1beta9 had yet to be
released. To fix this, I've removed v1beta10 and merged the original
v1beta9->v1beta10 upgrade  into the current v1beta8->v1beta9 upgrade.
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