Service and Revision labels #342

aaron-lerner · 2019-08-06T22:39:56Z

Fixes #328

Proposed Changes

Adds a --label (alias -l) flag to set service and revision template labels
Allow unsetting env vars and labels

Release Note

Added --label (alias -l) on `kn service create` and `kn service update` commands to set service and revision template labels.

knative-prow-robot

@aaron-lerner: 1 warning.

In response to this:

Partially addresses #328

Proposed Changes

Adds a --label flag to set service-level labels

Release Note
Add --label (alias -l) on `kn service create` and `kn service update` commands to set service-level labels.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/serving/config_changes.go

knative-prow-robot · 2019-08-06T22:40:11Z

Hi @aaron-lerner. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sixolet · 2019-08-06T23:15:15Z

/ok-to-test

pkg/kn/commands/service/configuration_edit_flags.go

sixolet

Looking promising, just a few small suggestions.

pkg/kn/commands/service/configuration_edit_flags.go

pkg/serving/config_changes.go

pkg/kn/commands/service/configuration_edit_flags.go

pkg/kn/commands/service/service_create_test.go

duglin · 2019-08-06T23:30:19Z

overall looks good. Did you want to support unsetting a label too?

aaron-lerner · 2019-08-07T02:09:26Z

Added support for unsetting by passing an empty value, ie --label foo= will remove the label named foo.

rhuss · 2019-08-07T09:41:37Z

Added support for unsetting by passing an empty value, ie --label foo= will remove the label named foo.

Does the env var support this, too ? It should be treated the same imo. AFAIK, empty label values, are allowed, too, so not sure how this could be achieved if an empty value means to remove a label.

rhuss · 2019-08-07T09:43:50Z

I'm also wondering whether how we could allow setting labels on services, but also on the pod templates. I think both might be useful, and I wonder how the UX for this would look like.

--labels for service labels
--pod-labels (?) for labels on pods

Or should --labels set the label on both ? Would be the simplest and maybe that's already good enough ?

rhuss · 2019-08-07T09:46:10Z

~~I see service labels are already applied to the generated pods (if I understand that correctly :). So then please forget my last comment :)~~

Nope, doesn't seem so. So my last question still holds: Apply the label to service itself or the contained the revision template ? I would argue for the latter or both

rhuss · 2019-08-07T10:33:56Z

BTW, you see how a revision get the labels from the template, not the sevice itself here: https://github.com/knative/serving/blob/c6860bf7c0663ddbf71d8fa3b48cbfbd9b54c205/pkg/reconciler/configuration/resources/revision.go#L30-L33

aaron-lerner · 2019-08-08T01:29:34Z

Changed the unset behavior to match kubectl. So pass the name/key of the environment variable followed by - to unset, e.g. --env NAME- unsets the environment variable with name NAME, same behavior for --label. Let me know if you'd like the changes to --env to be moved out into another PR, I don't think all the changes necessarily need to be entangled in this one, although they are heavily related.

With this, you can now set a label with an empty string and the already existing behavior of setting an environment variable with no value still works.

rhuss · 2019-08-08T07:25:37Z

--env KEY=value sets EnvVar{Name: "KEY", Value: "value"} (current behavior)

👍

--env KEY= sets EnvVar{Name: "KEY"} (current behavior)

--env KEY invalid

I would treat both variants as the same, no need to make --env KEY invalid imo.

--env KEY- removes the env var matching EnvVar{Name: "KEY", ...}

👍 as this is similar to the the kubectl behaviour.

--env KEY-= sets EnvVar{Name: "KEY-"} (current behavior)

This is an invalid env var name, an env var can't end with a -. I would just treat it the same as above, i.e. deleting the env variable.

rhuss

thanks a lot, very nice PR, love it :)

'happy to update the env handling in one sweep here, too.

See some comments inline, the biggest thing that we still have to decide which label to update (i opt for both locations).

pkg/kn/commands/parsing_helper.go

pkg/kn/commands/parsing_helper_test.go

pkg/kn/commands/service/service_create_test.go

rhuss · 2019-08-08T09:01:19Z

pkg/serving/config_changes.go

+// UpdateServiceLabels updates the labels on a service
+func UpdateServiceLabels(service *servingv1alpha1.Service, vars map[string]string) error {
+	if service.ObjectMeta.Labels == nil {
+		service.ObjectMeta.Labels = make(map[string]string)


It seems at that point there might as well be separate --revision-label, --service-label , and --label flags. It would be much more explicit both in the documentation and I think code that way. Plus, special characters change (slightly) across different shell environments and if this list is any indication, it may not be worth the time and effort to find and confirm which characters are valid in a reasonable number of shell environments.

This is always a tradeoff between options bloat and being too obfuscated. Currently, there is a tendency to add 1-5 new options per new features, and I'm doing my best to fight against the bloat. At least asking, whether there could be a compromise and whether all those options are needed (e.g. see #345 (comment) or #326 (review) or #282 (review) how to reduce the UX surface and so the cognitive load).

Of course, we should no go over the top, and using special chars like that might be the case here (although ^ and : are safe shell chars so no need to escape).

What's about that compromise: Let's implement --label to label both, service and template and wait on feedback if the other use cases (individual labelling of serving/template) are actually requested ?

From a single feature's PR POV adding 3 new options might not be a big deal, but its the sum which might lead to multi page help outputs which are really overwhelming (especially when alphabetically ordered, and --revision-label, --sevice-label and --label will apear on totally different positions in a list of 50+ options)

pkg/serving/config_changes.go

pkg/serving/config_changes_test.go

navidshaikh · 2019-08-08T10:05:56Z

--env KEY=value sets EnvVar{Name: "KEY", Value: "value"} (current behavior)
--env KEY= sets EnvVar{Name: "KEY"} (current behavior)
--env KEY- removes the env var matching EnvVar{Name: "KEY", ...}

+1 for support these operations.

--env KEY invalid

We should treat this same as --env KEY=. I agree with @rhuss .

aaron-lerner · 2019-08-08T16:37:47Z

This is an invalid env var name, an env var can't end with a -. I would just treat it the same as above, i.e. deleting the env variable.

I assumed the lack of validation client-side on names and values meant this was being done server-side. However, I am allowed to set an environment variable with a name ending in -. Are you suggesting adding client-side validation to names in this PR? I'm afraid something like that starts to bloat it a bit too much.

rhuss · 2019-08-08T17:12:24Z

I assumed the lack of validation client-side on names and values meant this was being done server-side. However, I am allowed to set an environment variable with a name ending in -. Are you suggesting adding client-side validation to names in this PR? I'm afraid something like that starts to bloat it a bit too much.

I just meant that --env VAR- and --env VAR-= should be treated the same, i.e. deleting the variable. No need to do further validation: If the var ends with - then delete it (if present, otherwise ignore)

aaron-lerner · 2019-08-08T22:58:59Z

Updated to have env and label keys passed without values map to empty values rather than throw an error. Additionally, keys that indicate deletion (- suffix) will always attempt to remove the associated env/label, regardless of whether or not a value is included with them.

maximilien · 2019-08-09T19:59:13Z

/retest

knative-metrics-robot · 2019-08-09T20:12:38Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	77.5%	79.5%	2.1
pkg/serving/config_changes.go	75.8%	81.0%	5.2
pkg/util/parsing_helper.go	Do not exist	100.0%

knative-prow-robot · 2019-08-13T23:41:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-lerner, maximilien, sixolet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,sixolet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sixolet · 2019-08-13T23:41:42Z

/lgtm

* Fix backup Makefile and auth Apparently "works on my machine" isn't enough. 1. The Makefile was broken, use the current directory for the build context. 2. Activate the service account (not necessary for gcrane, but necessary for gsutil). * Get service account key from env * For backup image, pass keyfile as first argument

aaron-lerner added 2 commits August 6, 2019 15:27

Add support for service labels with --label or -l

b9c6e52

Add tests

158202f

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 6, 2019

knative-prow-robot requested review from mattmoor and maximilien August 6, 2019 22:40

knative-prow-robot reviewed Aug 6, 2019

View reviewed changes

pkg/serving/config_changes.go Outdated Show resolved Hide resolved

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 6, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 6, 2019

Fix typo

ac92237

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2019

duglin reviewed Aug 6, 2019

View reviewed changes

pkg/kn/commands/service/configuration_edit_flags.go Outdated Show resolved Hide resolved

sixolet suggested changes Aug 6, 2019

View reviewed changes

pkg/kn/commands/service/configuration_edit_flags.go Outdated Show resolved Hide resolved

pkg/serving/config_changes.go Show resolved Hide resolved

duglin reviewed Aug 6, 2019

View reviewed changes

pkg/kn/commands/service/configuration_edit_flags.go Outdated Show resolved Hide resolved

duglin reviewed Aug 6, 2019

View reviewed changes

pkg/kn/commands/service/service_create_test.go Outdated Show resolved Hide resolved

mattmoor removed their request for review August 7, 2019 00:43

aaron-lerner added 6 commits August 6, 2019 18:27

Move MapToArray helper method to separate file

f1f0147

Allow unsetting labels by passing empty value

b9992db

Fix test

578a4d5

Add test case for label removal

664f703

Wrap error message with flag

8cc9d57

Update docs to include --label

2a3fb20

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019

aaron-lerner added 4 commits August 7, 2019 16:45

Update labels on both services and revisions

7cd588e

Add some test cases around weirder user input

6ba099b

Change unset behavior and allow setting empty env and label

da2029d

Update docs for new unset behavior

202f086

rhuss requested changes Aug 8, 2019

View reviewed changes

rhuss mentioned this pull request Aug 8, 2019

Supports setting comma separated env vars #347

Closed

aaron-lerner added 3 commits August 8, 2019 14:20

Make single keys to map to empty values

3f7d0ca

Move helper to util

29dc7d5

Use assert.DeepEqual

a962ae0

Use new mock test + check both service and revision

a80f54f

aaron-lerner changed the title ~~Service labels~~ Service and Revision labels Aug 9, 2019

aaron-lerner added 2 commits August 9, 2019 13:05

Merge remote-tracking branch 'upstream/master' into service-labels

4a9c6da

Use new method and correct year

dc17266

sixolet approved these changes Aug 13, 2019

View reviewed changes

knative-prow-robot assigned sixolet Aug 13, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2019

knative-prow-robot merged commit 27d8f43 into knative:master Aug 14, 2019

dsimansk added a commit to dsimansk/client that referenced this pull request Feb 6, 2024

[release-v1.11] Add kn-plugin-source-kafka v1.11.2 (knative#342)

fb86f4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service and Revision labels #342

Service and Revision labels #342

aaron-lerner commented Aug 6, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Aug 6, 2019

sixolet commented Aug 6, 2019

sixolet left a comment

duglin commented Aug 6, 2019

aaron-lerner commented Aug 7, 2019

rhuss commented Aug 7, 2019

rhuss commented Aug 7, 2019

rhuss commented Aug 7, 2019 •

edited

Loading

rhuss commented Aug 7, 2019

aaron-lerner commented Aug 8, 2019

rhuss commented Aug 8, 2019 •

edited

Loading

rhuss left a comment

rhuss Aug 8, 2019 •

edited

Loading

navidshaikh commented Aug 8, 2019

aaron-lerner commented Aug 8, 2019

rhuss commented Aug 8, 2019

aaron-lerner commented Aug 8, 2019

maximilien commented Aug 9, 2019

knative-metrics-robot commented Aug 9, 2019

knative-prow-robot commented Aug 13, 2019

sixolet commented Aug 13, 2019

Service and Revision labels #342

Service and Revision labels #342

Conversation

aaron-lerner commented Aug 6, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Aug 6, 2019

sixolet commented Aug 6, 2019

sixolet left a comment

Choose a reason for hiding this comment

duglin commented Aug 6, 2019

aaron-lerner commented Aug 7, 2019

rhuss commented Aug 7, 2019

rhuss commented Aug 7, 2019

rhuss commented Aug 7, 2019 • edited Loading

rhuss commented Aug 7, 2019

aaron-lerner commented Aug 8, 2019

rhuss commented Aug 8, 2019 • edited Loading

rhuss left a comment

Choose a reason for hiding this comment

rhuss Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

navidshaikh commented Aug 8, 2019

aaron-lerner commented Aug 8, 2019

rhuss commented Aug 8, 2019

aaron-lerner commented Aug 8, 2019

maximilien commented Aug 9, 2019

knative-metrics-robot commented Aug 9, 2019

knative-prow-robot commented Aug 13, 2019

sixolet commented Aug 13, 2019

aaron-lerner commented Aug 6, 2019 •

edited

Loading

rhuss commented Aug 7, 2019 •

edited

Loading

rhuss commented Aug 8, 2019 •

edited

Loading

rhuss Aug 8, 2019 •

edited

Loading