Prefix Skaffold labels with 'skaffold-' #2062

charlyx · 2019-05-03T14:14:48Z

Hi 👋

First of all, thanks for your work!

I hope this PR meets #2002 expectations 😄

Thanks to @pyaillet for his help 🤗

googlebot · 2019-05-03T14:14:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

charlyx · 2019-05-03T14:16:26Z

I signed it!

googlebot · 2019-05-03T14:16:33Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

corneliusweig · 2019-05-03T15:35:55Z

I'm a bit worried that somebody may forget to add the skaffold- prefix when this issue has been forgotten. I would prefer, if the labeler would check each label whether it needs to be prefixed. WDYT?

tejal29 · 2019-05-03T18:11:01Z

pkg/skaffold/config/options_test.go

-				"two":     "second",
-				"three":   "",
-				"four":    "",
+				"cleanup":          "true",


why cleanup and skaffold-cleanup both exist here?

If I understand correctly, in #2002, it's question of keeping both for sometimes to make sure not to break potential use from skaffold users.
If this is excessive precaution, we might just replace the existing ones.

ah understood. this is for backward compatibility. However these are labels that skaffold uses internally to track pods that needed to cleaned-up. Users should not have to care about these labels.
So. removing them should be fine.

tejal29 · 2019-05-03T18:14:44Z

I'm a bit worried that somebody may forget to add the skaffold- prefix when this issue has been forgotten. I would prefer, if the labeler would check each label whether it needs to be prefixed. WDYT?

Thats is a great thought. The labeller could have something is "AddSkaffoldPrefixIfRequired". However there is an exception to this rule when you want to add k8 recommended style labels.
Maybe we could do some more strict validations like if its one of the recommended style label or not?

corneliusweig · 2019-05-03T18:39:03Z

However there is an exception to this rule when you want to add k8 recommended style labels .

@tejal29 good point. So at the very least, we should check if the label already has a app.kubernetes.io prefix, and if it does skip adding our prefix.

Further, it seems to be common convention to delimit label namespaces with a /. Therefore, if a label already contains a /, we should assume that the label is already namespaced and skip the skaffold prefix. WDYT?

Now that I think about it, should we not use a more expressive prefix than skaffold-? My favorite would be skaffold.dev/, e.g. skaffold.dev/tail. That way, a completely oblivious user would be directed to the skaffold web page.

pyaillet · 2019-05-03T19:42:56Z

@corneliusweig I like the idea of a skaffold.dev/ prefix.
However, what should be done of user label passed with skaffold ... -l userlabel=uservalue ?

corneliusweig · 2019-05-03T20:00:25Z

@pyaillet good catch! I as a user would be surprised, if my label was prefixed. So I would say, no prefix for any explicitly user-specified labels. Or what is your feeling?

corneliusweig · 2019-05-03T20:07:00Z

@charlyx there is one contrived piece of code where the prefixing everything is not correct. The helm deployer needs to apply labels after creating the objects in k8s. For that, the labeler needs to merge existing and new skaffold labels. Those existing labels should of course not be prefixed with skaffold... .

pyaillet · 2019-05-03T22:01:57Z

I see three options there:

Add a AddSkaffoldPrefixIfRequired method and call it where merge is called. However user labels would be prefixed, which is not really satisfying
Add the AddSkaffoldPrefixIfRequired method, call it there, just before user labels handling. Keeping prefix hardcoded here
Create a way to distinguish between ToBePrefixedLabels and ToBeKeptLabels by extending the Labeller interface, but this one seems a bit overengineered to me

I think the second might be a good option, WDYT ?

tejal29 · 2019-05-03T22:11:47Z

Thanks @pyaillet i feel we should go with your current implementation and enforce "skaffold-" prefix via code reviews and not use AddSkaffoldPrefixIfRequired at all. We don't want to add too many branches to the flow and add more complexity.

googlebot · 2019-05-03T22:44:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

pyaillet · 2019-05-03T22:45:55Z

I signed it!
And I'm ok with my commits being contributed to this project.

codecov-io · 2019-05-03T22:57:47Z

Codecov Report

Merging #2062 into master will not change coverage.
The diff coverage is 75%.

@@           Coverage Diff           @@
##           master    #2062   +/-   ##
=======================================
  Coverage   56.12%   56.12%           
=======================================
  Files         180      180           
  Lines        7753     7753           
=======================================
  Hits         4351     4351           
  Misses       2986     2986           
  Partials      416      416

Impacted Files	Coverage Δ
pkg/skaffold/deploy/labels.go	`16.04% <ø> (ø)`	⬆️
pkg/skaffold/config/options.go	`90.47% <75%> (ø)`	⬆️

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 25f1b66...b843c51. Read the comment docs.

charlyx · 2019-05-04T16:52:10Z

I'm ok with my commits being contributed to this project.

corneliusweig

As it will be the task of the reviewer to ensure proper label prefixes, you could add a godoc comment to the Labeller interface (pkg/skaffold/deploy/labels.go) with a reminder that labels should be namespaced appropriately.

pkg/skaffold/config/options.go

tejal29 · 2019-05-06T18:09:24Z

@pyaillet Looks like you dont have the CLA signed for the following email id.

pyaillet · 2019-05-06T18:35:16Z

I signed it!

pyaillet · 2019-05-06T18:37:05Z

Really strange, when I got to the CLA page I got:
"It looks like you've already signed this CLA. If you'd like to edit your contact information, you may do so below."
Sorry for the troubles :\

pyaillet · 2019-05-06T18:37:57Z

Is it related to this comment from GoogleBot ?

charlyx · 2019-05-06T18:47:10Z

Please have them confirm that here in the pull request

We have @tejal29 :)

…

Sent from my iPhone

On 6 May 2019, at 8:38 PM, Pierre-Yves Aillet ***@***.***> wrote: Is it related to this comment from GoogleBot ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

priyawadhwa · 2019-05-06T18:50:42Z

I think the CLA has been signed, looks like it just hasn't updated on the PR @tejal29

googlebot · 2019-05-06T22:16:55Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

tejal29 · 2019-05-06T22:17:29Z

Thanks @pyaillet and @priyawadhwa. I forced a Re-Scan and the error is "CLAs are signed, but unable to verify author consent" which is terminal
So i have to manually remove the label.
Thanks

Jonathan34 · 2019-05-08T16:04:59Z

Is it possible that this broke CI/CD:

Resource: "networking.istio.io/v1alpha3, Resource=virtualservices", GroupVersionKind: "networking.istio.io/v1alpha3, Kind=VirtualService"
Name: "istio-routes", Namespace: "development"
Object: &{map["apiVersion":"networking.istio.io/v1alpha3" "kind":"VirtualService" "metadata":map["annotations":map["kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"networking.istio.io/v1alpha3\",\"kind\":\"VirtualService\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/managed-by\":\"skaffold-f25b09f\",\"environment\":\"development\",\"skaffold.dev/builder\":\"google-cloud-build\",\"skaffold.dev/cleanup\":\"true\",\"skaffold.dev/deployer\":\"kustomize\",\"skaffold.dev/profiles\":\"gcb\",\"skaffold.dev/tag-policy\":\"git-commit\",\"skaffold.dev/tail\":\"true\"},\"name\":\"istio-routes\",\"namespace\":\"development\"},\"spec\":{<redacted>}}\n"] "creationTimestamp":"2019-04-29T15:32:45Z" "generation":'\x01' "labels":map["app.kubernetes.io/managed-by":"skaffold-f25b09f" "environment":"development" "skaffold.dev/builder":"google-cloud-build" "skaffold.dev/cleanup":"true" "skaffold.dev/deployer":"kustomize" "skaffold.dev/profiles":"gcb" "skaffold.dev/tag-policy":"git-commit" "skaffold.dev/tail":"true"] "name":"istio-routes" "namespace":"development" "resourceVersion":"20715208" "selfLink":"/apis/networking.istio.io/v1alpha3/namespaces/development/virtualservices/istio-routes" "uid":"090cfcd6-6a94-11e9-af1e-42010a800034"] "spec":map[<redacted>]]}
for: "STDIN": VirtualService.networking.istio.io "istio-routes" is invalid: metadata.labels: Invalid value: "skaffold-": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')
time="2019-05-08T15:10:51Z" level=fatal msg="deploy failed: kubectl error: kubectl apply: exit status 1"

pyaillet · 2019-05-08T16:20:47Z

As the prefix set here is skaffol.dev/, I would say no

tejal29 · 2019-05-08T16:36:36Z

Looks like something else might be adding the label.
Can you provide logs of skaffold run with --verbosity=debug ?

pyaillet · 2019-05-08T17:30:39Z

It's strange, the only label value prefixed by skaffold- in the logs is: "app.kubernetes.io/managed-by":"skaffold-f25b09f"

tejal29 · 2019-05-08T17:37:20Z

yeah looks like the version is not get populated. :( maybe we shd fix this to check if version is not empty.

corneliusweig · 2019-05-08T18:07:53Z

But the version string (albeit weird) is not the problem, as it fits the regexp.

@Jonathan34 I think you are using kustomize to deploy, correct? Can you check if you do some label magic there? And are you using the AdmissionWebhookController or do you call istioctl manually? Maybe the clue as to what is going on is hidden there.

Jonathan34 · 2019-05-08T18:10:55Z

Looks like something else might be adding the label.
Can you provide logs of skaffold run with --verbosity=debug ?

is there a good place i can confidentially share the logs?
I am running skaffold from the latest skaffold docker image. the deploy step is a kustomize overlay that worked yesterday morning (probably should not use skaffold:latest :P)

The guilty label seems to be the following:
app.kubernetes.io/managed-by: skaffold-

the tag policy i set is:

 tagPolicy:
    gitCommit: {}

here is an abstract of the log:

Generating tags...
time="2019-05-08T17:03:56Z" level=debug msg="Running command: [git describe --tags --always]"
 - gcr.io/<redacted>/myservice -> time="2019-05-08T17:03:56Z" level=info msg="update check failed: get latest and current Skaffold version: parsing current semver, skipping update check: parsing semver: Version string empty"
time="2019-05-08T17:03:57Z" level=debug msg="Command output: [62577b7\n]"
time="2019-05-08T17:03:57Z" level=debug msg="Running command: [git status . --porcelain]"
gcr.io/<redacted>/myservice:62577b7
Tags generated in 1.4113427s

...
Build complete in 2m20.0544816s
Starting test...
Test complete in 20.9µs
Starting deploy...
kubectl client version: 1.14

But the version string (albeit weird) is not the problem, as it fits the regexp.

@Jonathan34 I think you are using kustomize to deploy, correct? Can you check if you do some label magic there? And are you using the AdmissionWebhookController or do you call istioctl manually? Maybe the clue as to what is going on is hidden there.

actually the skaffold- fails the regexp as it does not end with alpha numerical char.
I use kustomize.
I am not using any AdmissionWebhookController,
i am not calling istioctl manually. ihave the istio files listed in the kustomization resources so they are applied everytime with the overlays.

One overlay i have looks like this:

commonLabels:
  environment: development
bases:
  - ../../base
namespace: development
patchesStrategicMerge:
  - patches/istio-routes.yaml
  - patches/istio-whitelist-cloudsql-app-models-egress.yaml
patchesJson6902:
  - target:
      group: networking.istio.io
      version: v1alpha3
      kind: VirtualService
      name: istio-routes-ui
    path: patches/routes-ui-patch.yaml

I am willing to show my config to someone in a video conf or anything, but i can't send the whole package. Or I need to switch to the google enterprise support portal...

corneliusweig · 2019-05-08T18:17:51Z

I am running skaffold from the latest skaffold docker image

I take it you are talking about gcr.io/k8s-skaffold/skaffold:latest?

Jonathan34 · 2019-05-08T18:24:01Z

I am running skaffold from the latest skaffold docker image

I take it you are talking about gcr.io/k8s-skaffold/skaffold:latest?

yes;

Using default tag: latest
latest: Pulling from k8s-skaffold/skaffold
Digest: sha256:275894cbe96cbf2c5262bba710530acbad91bd8af329b6d6eea9ca6086d14642
Status: Image is up to date for gcr.io/k8s-skaffold/skaffold:latest

corneliusweig · 2019-05-08T18:38:24Z

So the skaffold image does have a version set (albeit a strange one):

docker run --rm -ti gcr.io/k8s-skaffold/skaffold:latest skaffold version \
  -o "{{.Version}}  {{.GitCommit}} {{.GitTreeState}}"

yields

a117236  a1172365f8c099635277a46c20acfa1e1d6ed2a7 clean%

Overall, to me it does not look like DefaultLabeller would cause the bad label.

tejal29 · 2019-05-08T18:45:42Z

umm we shd fix that. Sorry about it.

charlyx requested review from balopat, dgageot, nkubala, priyawadhwa and tejal29 as code owners May 3, 2019 14:14

googlebot added the cla: no label May 3, 2019

googlebot added cla: yes and removed cla: no labels May 3, 2019

tejal29 reviewed May 3, 2019

View reviewed changes

charlyx and others added 2 commits May 4, 2019 00:30

Prefix Skaffold labels with 'skaffold-'

90d24ad

👌 fixes after review

255e2e2

pyaillet force-pushed the issue-2002 branch from 06d23aa to 255e2e2 Compare May 3, 2019 22:44

googlebot added cla: no and removed cla: yes labels May 3, 2019

corneliusweig suggested changes May 5, 2019

View reviewed changes

pkg/skaffold/config/options.go Outdated Show resolved Hide resolved

Replace prefix skaffold- with skaffold.dev/

b843c51

tejal29 approved these changes May 6, 2019

View reviewed changes

tejal29 added the kokoro:run runs the kokoro jobs on a PR label May 6, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 6, 2019

corneliusweig approved these changes May 6, 2019

View reviewed changes

tejal29 added the cla: yes label May 6, 2019

tejal29 removed the cla: no label May 6, 2019

tejal29 merged commit 0558713 into GoogleContainerTools:master May 6, 2019

charlyx deleted the issue-2002 branch May 7, 2019 07:19

tejal29 mentioned this pull request May 8, 2019

skaffold image does not have version. #2093

Closed

corneliusweig mentioned this pull request May 27, 2019

Should all Skaffold labels be prefixed with skaffold-? #2002

Closed

Prefix Skaffold labels with 'skaffold-' #2062

Prefix Skaffold labels with 'skaffold-' #2062

Conversation

charlyx commented May 3, 2019 • edited Loading

googlebot commented May 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

charlyx commented May 3, 2019

googlebot commented May 3, 2019

corneliusweig commented May 3, 2019

tejal29 May 3, 2019

Choose a reason for hiding this comment

pyaillet May 3, 2019

Choose a reason for hiding this comment

tejal29 May 3, 2019

Choose a reason for hiding this comment

tejal29 commented May 3, 2019

corneliusweig commented May 3, 2019

pyaillet commented May 3, 2019

corneliusweig commented May 3, 2019 • edited Loading

corneliusweig commented May 3, 2019

pyaillet commented May 3, 2019 • edited Loading

tejal29 commented May 3, 2019 • edited Loading

googlebot commented May 3, 2019

pyaillet commented May 3, 2019 • edited Loading

codecov-io commented May 3, 2019 • edited Loading

Codecov Report

charlyx commented May 4, 2019

corneliusweig left a comment

Choose a reason for hiding this comment

tejal29 commented May 6, 2019 • edited Loading

pyaillet commented May 6, 2019

pyaillet commented May 6, 2019

pyaillet commented May 6, 2019

charlyx commented May 6, 2019 via email

priyawadhwa commented May 6, 2019

googlebot commented May 6, 2019

tejal29 commented May 6, 2019

Jonathan34 commented May 8, 2019

pyaillet commented May 8, 2019

tejal29 commented May 8, 2019

pyaillet commented May 8, 2019 • edited Loading

tejal29 commented May 8, 2019

corneliusweig commented May 8, 2019

Jonathan34 commented May 8, 2019 • edited Loading

corneliusweig commented May 8, 2019

Jonathan34 commented May 8, 2019 • edited Loading

corneliusweig commented May 8, 2019 • edited Loading

tejal29 commented May 8, 2019

charlyx commented May 3, 2019 •

edited

Loading

corneliusweig commented May 3, 2019 •

edited

Loading

pyaillet commented May 3, 2019 •

edited

Loading

tejal29 commented May 3, 2019 •

edited

Loading

pyaillet commented May 3, 2019 •

edited

Loading

codecov-io commented May 3, 2019 •

edited

Loading

tejal29 commented May 6, 2019 •

edited

Loading

pyaillet commented May 8, 2019 •

edited

Loading

Jonathan34 commented May 8, 2019 •

edited

Loading

Jonathan34 commented May 8, 2019 •

edited

Loading

corneliusweig commented May 8, 2019 •

edited

Loading