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 labels from builders and deployers #4485

Closed
wants to merge 1 commit into from

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jul 16, 2020

fixes #3133
second attempt after #4466

this removes all labels set by skaffold except for skaffold.dev/run-id, and app.kubernetes.io/managed-by=skaffold. this does remove the version from the managed-by label though, so changing skaffold version will not cause issues.

because we don't apply labels from build or deploy, we can remove this from their interfaces, as well as remove passing an array of Labellers to Deploy and Render. the labels are computed once when the runner is created, and stored inside the deployer.

@nkubala nkubala requested a review from a team as a code owner July 16, 2020 22:02
@nkubala nkubala requested a review from dgageot July 16, 2020 22:02
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #4485 into master will increase coverage by 0.15%.
The diff coverage is 82.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4485      +/-   ##
==========================================
+ Coverage   72.26%   72.42%   +0.15%     
==========================================
  Files         331      333       +2     
  Lines       12863    12886      +23     
==========================================
+ Hits         9296     9333      +37     
+ Misses       2975     2957      -18     
- Partials      592      596       +4     
Impacted Files Coverage Δ
pkg/skaffold/build/build.go 0.00% <ø> (ø)
pkg/skaffold/build/cluster/types.go 100.00% <ø> (ø)
pkg/skaffold/build/gcb/types.go 38.46% <ø> (+7.21%) ⬆️
pkg/skaffold/build/local/types.go 86.20% <ø> (+16.76%) ⬆️
pkg/skaffold/build/tag/custom.go 60.00% <ø> (+22.50%) ⬆️
pkg/skaffold/build/tag/date_time.go 56.25% <ø> (+8.88%) ⬆️
pkg/skaffold/build/tag/env_template.go 100.00% <ø> (+12.50%) ⬆️
pkg/skaffold/build/tag/git_commit.go 90.16% <ø> (+4.22%) ⬆️
pkg/skaffold/build/tag/sha256.go 100.00% <ø> (+30.00%) ⬆️
pkg/skaffold/deploy/deploy.go 100.00% <ø> (ø)
... and 39 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 6af3198...806f9fb. Read the comment docs.

pkg/skaffold/deploy/helm_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kubectl.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kubectl_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kustomize.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kustomize_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kustomize_test.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kustomize_test.go Outdated Show resolved Hide resolved
@nkubala
Copy link
Contributor Author

nkubala commented Jul 17, 2020

@dgageot thanks for the review, I addressed your comments. travis is being silly today though

@nkubala
Copy link
Contributor Author

nkubala commented Jul 20, 2020

something very strange is going on with travis. seems like it's using an old commit, I think there might be a caching bug? tests are passing for me locally and the syntax errors it's giving in the linter/windows unit/integration partitions don't match up with my latest branch.

going to close this PR and reopen it as is 🤷

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.

Updating Skaffold breaks redeploys since it changes labels
3 participants