Don't add skaffold labels by default in render, and deprecate the --add-skaffold-labels flag #5653
Conversation
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
b41e6cf to
1c35477
Compare
Codecov Report
@@ Coverage Diff @@
## master #5653 +/- ##
========================================
Coverage 70.78% 70.79%
========================================
Files 432 436 +4
Lines 16242 16356 +114
========================================
+ Hits 11497 11579 +82
- Misses 3901 3923 +22
- Partials 844 854 +10
Continue to review full report at Codecov.
|
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
8573786 to
faa4377
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
faa4377 to
ff04697
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
ff04697 to
1cb5471
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
1cb5471 to
347d2b2
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
347d2b2 to
8e8c2e8
Compare
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
8e8c2e8 to
170efb9
Compare
|
@googlebot I consent |
|
this has got to be the most frustrating CI experience on a PR that i've ever had. i've probably hit the docker pull rate limit two dozen times on various Travis jobs here. |
cmd/skaffold/app/cmd/flags.go
Outdated
| "dev": true, | ||
| "debug": true, | ||
| "run": true, | ||
| }, | ||
| FlagAddMethod: "BoolVar", | ||
| DefinedOn: []string{"render"}, |
There was a problem hiding this comment.
You need to add dev, debug, and run here. It might make sense to invert the defaults so DevValuePerCommand = { "render": false }.
You should need to regenerate the man pages when changing the defaults.
There was a problem hiding this comment.
You need to add dev, debug, and run here.
I just updated this, but after pushing the change I'm thinking we don't actually want to define this for dev/debug/run here - do we really want this flag to be available for use on those commands? skaffold won't work correctly if the user messes with it.
It might make sense to invert the defaults so DevValuePerCommand = { "render": false }.
regardless of my above comment, agree with this
You should need to regenerate the man pages when changing the defaults.
I think we also don't need to do this, since this flag is Deprecated it won't show up in the help text.
…dd-skaffold-labels flag
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
11881af to
1fae845
Compare
nkubala
force-pushed
the
deprecate-render-skaffold-labels
branch
from
a5f1818 to
2222e3f
Compare
Fixes: #5542
Related: #5644 (both add the same logic for supporting
Deprecatedfield on flagsMerge before/after: #5644
Description
this change flips our default value for
--add-skaffold-labelsonskaffold renderto false. previously, theskaffold.dev/run-idandapp.kubernetes.io/managed-bylabels were being added to all manifests hydrated by Skaffold when this flag wasn't explicitly specified, but these labels provide no value to users.this change also deprecates this flag, since there isn't really a reason for users to be adding these labels to any hydrated resources through Skaffold.