Repository: knative/client
Pull request #670 (2020-02-17 08:07:54)

chore(e2e): Minor cleanup

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

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/670#" title="Author self-approved">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":[]} -->
knative-prow-robot: @rhuss: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 73ea059ff6659b755cb5e4e137d228ca61223711 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229316627502206977) | `/test pull-knative-client-integration-tests`

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

<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 -->
knative-prow-robot: @rhuss: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 73ea059ff6659b755cb5e4e137d228ca61223711 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229316627502206977) | `/test pull-knative-client-integration-tests`
pull-knative-client-integration-tests-latest-release | 73ea059ff6659b755cb5e4e137d228ca61223711 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests-latest-release/1229316627502206976) | `/test pull-knative-client-integration-tests-latest-release`

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

<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 -->
navidshaikh: /retest
knative-prow-robot: @rhuss: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 73ea059ff6659b755cb5e4e137d228ca61223711 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229332231600738305) | `/test pull-knative-client-integration-tests`

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

<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 -->
knative-prow-robot: @rhuss: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 30cce027bd628c795b332e361cbe9210ca272bf6 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229345065873182720) | `/test pull-knative-client-integration-tests`

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

<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 -->
navidshaikh: /retest

```
        🦆 kn service update hello --env TARGET=kn --port 8888 --namespace kne2etests0
        ┃ 
        🔥 Operation cannot be fulfilled on services.serving.knative.dev "hello": the object has been modified; please apply your changes to the latest version and try again
```
rhuss: >   🦆 kn service update hello --env TARGET=kn --port 8888 --namespace kne2etests0
>         ┃ 
>         🔥 Operation cannot be fulfilled on services.serving.knative.dev "hello": the object has been modified; please apply your changes to the latest version and try again

Hmm, I thought that we don't have that issue anymore as we are doing 3 retries to upate. Maybe I should add a sleep of 1s to the retries ?
knative-prow-robot: @rhuss: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 30cce027bd628c795b332e361cbe9210ca272bf6 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229357396451659776) | `/test pull-knative-client-integration-tests`

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

<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 -->
dsimansk: /retest

```
conversion webhook for &{map[apiVersion:serving.knative.dev/v1 kind:Service metadata:map[creationTimestamp:2020-02-17T11:16:41Z generation:1 name:svc1 namespace:kne2esmoketests uid:5082bda3-2456-4512-ba0b-43b246293519] spec:map[template:map[metadata:map[annotations:map[client.knative.dev/user-image:gcr.io/knative-samples/helloworld-go] creationTimestamp:<nil> name:svc1-tbllh-1] spec:map[containers:[map[env:[map[name:TARGET value:Knative]] image:gcr.io/knative-samples/helloworld-go name: resources:map[]]]]]]]} failed: Post https://webhook.knative-serving.svc:443/?timeout=30s: no endpoints available for service "webhook"
```
knative-prow-robot: @rhuss: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 30cce027bd628c795b332e361cbe9210ca272bf6 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229372872921714688) | `/test pull-knative-client-integration-tests`

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

<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 -->
navidshaikh: /retest

```
        🦆 kn service create hello --image gcr.io/knative-samples/helloworld-go --namespace kne2etests8
        ┃ Creating service 'hello' in namespace 'kne2etests8':
        ┃ 
        ┃   1.617s The Route is still working to reflect the latest desired specification.
        ┃   2.029s Revision "hello-pvrmq-1" referenced in traffic not found.
        ┃ 
        🔥 RevisionMissing: Revision "hello-pvrmq-1" referenced in traffic not found.
```

@rhuss :
> Maybe I should add a sleep of 1s to the retries ?

We can try that. 

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

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | 30cce027bd628c795b332e361cbe9210ca272bf6 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229397662546006016) | `/test pull-knative-client-integration-tests`

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

<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 -->
navidshaikh: /retest

```
        traffic_split_test.go:417: assertion failed: error is not nil: expecting 2 targets, got 1 targets targets: ,echo2-rev-2,100,true separator: |
```
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/v1/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-go-coverage/1229440821875445764/artifacts/line-cov.html#file0) | 73.8% | 73.3% | -0.5

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

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-build-tests | 85609243474648db8b61a2b22c127fcf038bf133 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-build-tests/1229440821875445762) | `/test pull-knative-client-build-tests`

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

<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 -->
knative-prow-robot: @rhuss: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-build-tests | 85609243474648db8b61a2b22c127fcf038bf133 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-build-tests/1229440821875445762) | `/test pull-knative-client-build-tests`
pull-knative-client-integration-tests | 85609243474648db8b61a2b22c127fcf038bf133 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-integration-tests/1229440821875445763) | `/test pull-knative-client-integration-tests`

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

<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 -->
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/v1/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/670/pull-knative-client-go-coverage/1229449001682604033/artifacts/line-cov.html#file0) | 73.8% | 73.3% | -0.5


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)
diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go
index f337f66e..5cf19660 100644
--- a/pkg/serving/v1/client.go
+++ b/pkg/serving/v1/client.go
@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/fields"
 	"knative.dev/pkg/apis"
@@ -35,7 +36,7 @@ import (
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 	clientv1 "knative.dev/serving/pkg/client/clientset/versioned/typed/serving/v1"
 
-	"knative.dev/client/pkg/errors"
+	clienterrors "knative.dev/client/pkg/errors"
 )
 
 // Func signature for an updating function which returns the updated service object
@@ -162,7 +163,7 @@ func (cl *knServingClient) Namespace() string {
 func (cl *knServingClient) GetService(name string) (*servingv1.Service, error) {
 	service, err := cl.client.Services(cl.namespace).Get(name, v1.GetOptions{})
 	if err != nil {
-		return nil, errors.GetError(err)
+		return nil, clienterrors.GetError(err)
 	}
 	err = updateServingGvk(service)
 	if err != nil {
@@ -180,7 +181,7 @@ func (cl *knServingClient) WatchService(name string, timeout time.Duration) (wat
 func (cl *knServingClient) ListServices(config ...ListConfig) (*servingv1.ServiceList, error) {
 	serviceList, err := cl.client.Services(cl.namespace).List(ListConfigs(config).toListOptions())
 	if err != nil {
-		return nil, errors.GetError(err)
+		return nil, clienterrors.GetError(err)
 	}
 	serviceListNew := serviceList.DeepCopy()
 	err = updateServingGvk(serviceListNew)
@@ -204,7 +205,7 @@ func (cl *knServingClient) ListServices(config ...ListConfig) (*servingv1.Servic
 func (cl *knServingClient) CreateService(service *servingv1.Service) error {
 	_, err := cl.client.Services(cl.namespace).Create(service)
 	if err != nil {
-		return errors.GetError(err)
+		return clienterrors.GetError(err)
 	}
 	return updateServingGvk(service)
 }
@@ -241,9 +242,11 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU
 			// Retry to update when a resource version conflict exists
 			if apierrors.IsConflict(err) && retries < nrRetries {
 				retries++
+				// Wait a second before doing the retry
+				time.Sleep(time.Second)
 				continue
 			}
-			return err
+			return errors.Wrap(err, fmt.Sprintf("giving up after %d retries", nrRetries))
 		}
 		return nil
 	}
@@ -256,7 +259,7 @@ func (cl *knServingClient) DeleteService(serviceName string) error {
 		&v1.DeleteOptions{},
 	)
 	if err != nil {
-		return errors.GetError(err)
+		return clienterrors.GetError(err)
 	}
 
 	return nil
@@ -285,7 +288,7 @@ func (cl *knServingClient) GetConfiguration(name string) (*servingv1.Configurati
 func (cl *knServingClient) GetRevision(name string) (*servingv1.Revision, error) {
 	revision, err := cl.client.Revisions(cl.namespace).Get(name, v1.GetOptions{})
 	if err != nil {
-		return nil, errors.GetError(err)
+		return nil, clienterrors.GetError(err)
 	}
 	err = updateServingGvk(revision)
 	if err != nil {
@@ -351,7 +354,7 @@ func getBaseRevision(cl KnServingClient, service *servingv1.Service) (*servingv1
 func (cl *knServingClient) DeleteRevision(name string) error {
 	err := cl.client.Revisions(cl.namespace).Delete(name, &v1.DeleteOptions{})
 	if err != nil {
-		return errors.GetError(err)
+		return clienterrors.GetError(err)
 	}
 
 	return nil
@@ -361,7 +364,7 @@ func (cl *knServingClient) DeleteRevision(name string) error {
 func (cl *knServingClient) ListRevisions(config ...ListConfig) (*servingv1.RevisionList, error) {
 	revisionList, err := cl.client.Revisions(cl.namespace).List(ListConfigs(config).toListOptions())
 	if err != nil {
-		return nil, errors.GetError(err)
+		return nil, clienterrors.GetError(err)
 	}
 	return updateServingGvkForRevisionList(revisionList)
 }


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)
 	}
 }
 
diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go
index fd4aab9a..90f15c14 100644
--- a/test/e2e/traffic_split_test.go
+++ b/test/e2e/traffic_split_test.go
@@ -414,9 +414,6 @@ func (test *e2eTest) verifyTargets(t *testing.T, r *KnRunResultCollector, servic
 	out := test.serviceDescribeWithJsonPath(r, serviceName, targetsJsonPath)
 	assert.Check(t, out != "")
 	actualTargets, err := splitTargets(out, targetsSeparator, len(expectedTargets))
-	if err != nil {
-		r.AddDump("service", serviceName, test.namespace)
-	}
 	assert.NilError(t, err)
 	formattedActualTargets := formatActualTargets(t, actualTargets)
 	assert.DeepEqual(t, expectedTargets, formattedActualTargets)