Repository: knative/client
Pull request #672 (2020-02-17 15:40:49)

Prevent service-specific labels from being put on the revision template
See: https://github.com/knative/serving/pull/6865

Signed-off-by: Doug Davis <dug@us.ibm.com>

/lint


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/672#" title="Author self-approved">duglin</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/serving/config_changes.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/672/pull-knative-client-go-coverage/1229430630027104256/artifacts/line-cov.html#file0) | 79.9% | 79.9% | 0.1

knative-prow-robot: @duglin: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 2e5223936c7cdaa9f4cc22c169d3bd0570634f5c | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/672/pull-knative-client-integration-tests/1229430630031298561) | `/test pull-knative-client-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_client/672). [Your PR dashboard](https://gubernator.knative.dev/pr/duglin).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
duglin: /test pull-knative-client-integration-tests
rhuss: Thanks for the PR and this fix is a quick fix for this issue, but does not solve the underlying issue that we already discussed while implementing labels: How can a user specify to which object to apply the label ? The service or the template ? For now its both, but there are situation (like this) where we definitely want a more fine grained resolution.

So I'm +1 for this if we fix the other issue (like using `--label` for both , `--label-service` for only the service and `--label-pod` for the pod template. same for annotations.), too and then remove that workaround (because then the use can specify this on her own).

For this very specific label we will have direct support, too, so I don't think people will provided that as label anyway.

How does this sound ?
duglin: Do we know when https://github.com/knative/client/pull/629 will be merged? It seems to be taking a while and this is kind of a blocker for us. I'm ok with removing this one #629 is merged.
rhuss: @navid, that's a question to you --^ :)
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/672#" title="Author self-approved">duglin</a>*, *<a href="https://github.com/knative/client/pull/672#pullrequestreview-359872526" title="LGTM">rhuss</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/master/OWNERS)~~ [rhuss]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
duglin: Let me ask this... if we did the split label feature you mentioned... would we still want code to skip "known" labels that don't belong on Revisions if the user used `--label` ?  Might be good, dunno

Patch:
diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go
index 62d30dad..a265243b 100644
--- a/pkg/serving/config_changes.go
+++ b/pkg/serving/config_changes.go
@@ -362,6 +362,12 @@ func UpdateResources(template *servingv1.RevisionTemplateSpec, requestsResourceL
 	return nil
 }
 
+// ServiceOnlyLabels should only appear on the Service and NOT on the
+// Revision template
+var ServiceOnlyLabels = map[string]struct{}{
+	serving.GroupName + "/visibility": {},
+}
+
 // UpdateLabels updates the labels identically on a service and template.
 // Does not overwrite the entire Labels field, only makes the requested updates
 func UpdateLabels(service *servingv1.Service, template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
@@ -373,7 +379,11 @@ func UpdateLabels(service *servingv1.Service, template *servingv1.RevisionTempla
 	}
 	for key, value := range toUpdate {
 		service.ObjectMeta.Labels[key] = value
-		template.ObjectMeta.Labels[key] = value
+
+		// Only add it to the template if it's not in our ServiceOnly list
+		if _, ok := ServiceOnlyLabels[key]; !ok {
+			template.ObjectMeta.Labels[key] = value
+		}
 	}
 	for _, key := range toRemove {
 		delete(service.ObjectMeta.Labels, key)


Test patch:
diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go
index 82b12fd8..e43c27d0 100644
--- a/pkg/serving/config_changes_test.go
+++ b/pkg/serving/config_changes_test.go
@@ -331,6 +331,25 @@ func TestUpdateLabelsNew(t *testing.T) {
 		"a": "foo",
 		"b": "bar",
 	}
+	tLabels := labels // revision template labels
+
+	// Only test service-specific labels if we have any
+	if len(ServiceOnlyLabels) != 0 {
+		// Make a copy of the expected labels so we can modify the original
+		// list w/o changing what's expected for the revsion template
+		tLabels = map[string]string{}
+		for k, v := range labels {
+			tLabels[k] = v
+		}
+
+		// Just add a random value from the list to make sure it doesn't show
+		// up in the revision template
+		for k := range ServiceOnlyLabels {
+			labels[k] = "testing"
+			break
+		}
+	}
+
 	err := UpdateLabels(service, template, labels, []string{})
 	assert.NilError(t, err)
 
@@ -340,8 +359,8 @@ func TestUpdateLabelsNew(t *testing.T) {
 	}
 
 	actual = template.ObjectMeta.Labels
-	if !reflect.DeepEqual(labels, actual) {
-		t.Fatalf("Template labels did not match expected %v found %v", labels, actual)
+	if !reflect.DeepEqual(tLabels, actual) {
+		t.Fatalf("Template labels did not match expected %v found %v", tLabels, actual)
 	}
 }