Repository: knative/serving
Pull request #8956 (2020-08-07 05:38:06)

Remove the unused 'name' elements.
Also use existing const and optimize Replace call.

/assign @julz @dprotaso mattmoor

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/8956#" title="Author self-approved">vagababov</a>*

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

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:

- ~~[pkg/reconciler/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/OWNERS)~~ [vagababov]

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-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-serving-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/revision/revision.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/8956/pull-knative-serving-go-coverage/1291610354043850752/artifacts/line-cov.html#file1) | 97.9% | 97.8% | -0.1


Patch:
diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go
index ee53eb759..2a142eb85 100644
--- a/pkg/reconciler/revision/reconcile_resources.go
+++ b/pkg/reconciler/revision/reconcile_resources.go
@@ -97,10 +97,10 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision)
 			for _, status := range pod.Status.ContainerStatuses {
 				if status.Name == rev.Spec.GetContainer().Name {
 					if t := status.LastTerminationState.Terminated; t != nil {
-						logger.Infof("%s marking exiting with: %d/%s", rev.Name, t.ExitCode, t.Message)
+						logger.Infof("marking exiting with: %d/%s", t.ExitCode, t.Message)
 						rev.Status.MarkContainerHealthyFalse(v1.ExitCodeReason(t.ExitCode), v1.RevisionContainerExitingMessage(t.Message))
 					} else if w := status.State.Waiting; w != nil && hasDeploymentTimedOut(deployment) {
-						logger.Infof("%s marking resources unavailable with: %s: %s", rev.Name, w.Reason, w.Message)
+						logger.Infof("marking resources unavailable with: %s: %s", w.Reason, w.Message)
 						rev.Status.MarkResourcesAvailableFalse(w.Reason, w.Message)
 					}
 					break
@@ -128,7 +128,6 @@ func (c *Reconciler) reconcileImageCache(ctx context.Context, rev *v1.Revision)
 		} else if err != nil {
 			return fmt.Errorf("failed to get image cache %q: %w", imageName, err)
 		}
-
 	}
 	return nil
 }
@@ -163,7 +162,7 @@ func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 
 		want := pa.DeepCopy()
 		want.Spec = tmpl.Spec
-		if pa, err = c.client.AutoscalingV1alpha1().PodAutoscalers(pa.Namespace).Update(want); err != nil {
+		if pa, err = c.client.AutoscalingV1alpha1().PodAutoscalers(ns).Update(want); err != nil {
 			return fmt.Errorf("failed to update PA %q: %w", paName, err)
 		}
 	}
diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go
index 16f882395..90aa802f6 100644
--- a/pkg/reconciler/revision/revision.go
+++ b/pkg/reconciler/revision/revision.go
@@ -121,7 +121,6 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro
 				Name:        container.Name,
 				ImageDigest: digest,
 			}
-
 			return nil
 		})
 	}
@@ -137,25 +136,11 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgrec
 	readyBeforeReconcile := rev.IsReady()
 	c.updateRevisionLoggingURL(ctx, rev)
 
-	phases := []struct {
-		name string
-		f    func(context.Context, *v1.Revision) error
-	}{{
-		name: "image digest",
-		f:    c.reconcileDigest,
-	}, {
-		name: "user deployment",
-		f:    c.reconcileDeployment,
-	}, {
-		name: "image cache",
-		f:    c.reconcileImageCache,
-	}, {
-		name: "PA",
-		f:    c.reconcilePA,
-	}}
-
-	for _, phase := range phases {
-		if err := phase.f(ctx, rev); err != nil {
+	for _, phase := range []func(context.Context, *v1.Revision) error{
+		c.reconcileDigest, c.reconcileDeployment,
+		c.reconcileImageCache, c.reconcilePA,
+	} {
+		if err := phase(ctx, rev); err != nil {
 			return err
 		}
 	}
@@ -177,9 +162,7 @@ func (c *Reconciler) updateRevisionLoggingURL(ctx context.Context, rev *v1.Revis
 		return
 	}
 
-	uid := string(rev.UID)
-
-	rev.Status.LogURL = strings.Replace(
+	rev.Status.LogURL = strings.ReplaceAll(
 		config.Observability.LoggingURLTemplate,
-		"${REVISION_UID}", uid, -1)
+		"${REVISION_UID}", string(rev.UID))
 }


Test patch:
diff --git a/test/config/100-namespace.yaml b/test/config/100-namespace.yaml
index 801f6bbd8..b1414b416 100644
--- a/test/config/100-namespace.yaml
+++ b/test/config/100-namespace.yaml
@@ -26,3 +26,8 @@ apiVersion: v1
 kind: Namespace
 metadata:
   name: serving-tests-security
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: tls
diff --git a/test/e2e-auto-tls-tests.sh b/test/e2e-auto-tls-tests.sh
index 9e6f8b699..ee8043bc7 100755
--- a/test/e2e-auto-tls-tests.sh
+++ b/test/e2e-auto-tls-tests.sh
@@ -32,6 +32,8 @@ function setup_auto_tls_env_variables() {
 
   export CUSTOM_DOMAIN_SUFFIX="$(($RANDOM % 10000)).${E2E_PROJECT_ID}.${DOMAIN_NAME}"
 
+  export TLS_TEST_NAMESPACE="tls"
+
   local INGRESS_NAMESPACE=${GATEWAY_NAMESPACE_OVERRIDE}
   if [[ -z "${GATEWAY_NAMESPACE_OVERRIDE}" ]]; then
     INGRESS_NAMESPACE="istio-system"
@@ -75,18 +77,18 @@ function cleanup_auto_tls_common() {
   cleanup_custom_domain
 
   toggle_feature autoTLS Disabled config-network
-  kubectl delete kcert --all -n serving-tests
+  kubectl delete kcert --all -n "${TLS_TEST_NAMESPACE}"
 }
 
 function setup_http01_auto_tls() {
-    # The name of the test.
-  export AUTO_TLS_TEST_NAME="HTTP01"
-  # The name of the Knative Service deployed in Auto TLS E2E test.
-  export TLS_SERVICE_NAME="http01"
+    # The name of the test, lowercase to avoid hyphenation of the test name.
+  export AUTO_TLS_TEST_NAME="http01"
+  # Rely on the built-in naming (for logstream)
+  unset TLS_SERVICE_NAME
   # The full host name of the Knative Service. This is used to configure the DNS record.
-  export FULL_HOST_NAME="*.serving-tests.${CUSTOM_DOMAIN_SUFFIX}"
+  export FULL_HOST_NAME="*.${TLS_TEST_NAMESPACE}.${CUSTOM_DOMAIN_SUFFIX}"
 
-  kubectl delete kcert --all -n serving-tests
+  kubectl delete kcert --all -n "${TLS_TEST_NAMESPACE}"
 
   if [[ -z "${MESH}" ]]; then
     echo "Install cert-manager no-mesh ClusterIssuer"
@@ -105,7 +107,7 @@ function setup_selfsigned_per_ksvc_auto_tls() {
   # The name of the Knative Service deployed in Auto TLS E2E test.
   export TLS_SERVICE_NAME="self-per-ksvc"
 
-  kubectl delete kcert --all -n serving-tests
+  kubectl delete kcert --all -n "${TLS_TEST_NAMESPACE}"
   kubectl apply -f ${TMP_DIR}/test/config/autotls/certmanager/selfsigned/
 }
 
@@ -115,10 +117,10 @@ function setup_selfsigned_per_namespace_auto_tls() {
   # The name of the Knative Service deployed in Auto TLS E2E test.
   export TLS_SERVICE_NAME="self-per-namespace"
 
-  kubectl delete kcert --all -n serving-tests
+  kubectl delete kcert --all -n "${TLS_TEST_NAMESPACE}"
 
-  # Enable namespace certificate only for serving-tests namespaces
-  export NAMESPACE_WITH_CERT="serving-tests"
+  # Enable namespace certificate only for "${TLS_TEST_NAMESPACE}" namespaces
+  export NAMESPACE_WITH_CERT=""${TLS_TEST_NAMESPACE}""
   go run ./test/e2e/autotls/config/disablenscert
 
   kubectl apply -f ${TMP_DIR}/test/config/autotls/certmanager/selfsigned/
@@ -143,7 +145,7 @@ function cleanup_per_selfsigned_namespace_auto_tls() {
   echo "Uninstall namespace cert controller"
   kubectl delete -f ${SERVING_NSCERT_YAML} --ignore-not-found=true
 
-  kubectl delete kcert --all -n serving-tests
+  kubectl delete kcert --all -n "${TLS_TEST_NAMESPACE}"
   kubectl delete -f ${TMP_DIR}/test/config/autotls/certmanager/selfsigned/ --ignore-not-found=true
 }
 
diff --git a/test/e2e/autotls/auto_tls_test.go b/test/e2e/autotls/auto_tls_test.go
index 05ee6cedc..a3dbac427 100644
--- a/test/e2e/autotls/auto_tls_test.go
+++ b/test/e2e/autotls/auto_tls_test.go
@@ -48,7 +48,9 @@ type config struct {
 
 var env config
 
-func TestAutoTLS(t *testing.T) {
+// Technically TLS is proper Go casing for acronyms, but it makes our test naming
+// hyphenate between every character, and we hit length problems.
+func TestTls(t *testing.T) {
 	if err := envconfig.Process("", &env); err != nil {
 		t.Fatalf("Failed to process environment variable: %v.", err)
 	}
@@ -56,7 +58,7 @@ func TestAutoTLS(t *testing.T) {
 }
 
 func testAutoTLS(t *testing.T) {
-	clients := e2e.Setup(t)
+	clients := e2e.SetupWithNamespace(t, test.TLSNamespace)
 
 	names := test.ResourceNames{
 		Service: test.ObjectNameForTest(t),
@@ -93,11 +95,11 @@ func testAutoTLS(t *testing.T) {
 		}
 		if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, servingv1.RouteSpec{
 			Traffic: []servingv1.TrafficTarget{{
-				Tag:            "tag1",
+				Tag:            "r",
 				Percent:        ptr.Int64(50),
 				LatestRevision: ptr.Bool(true),
 			}, {
-				Tag:            "tag2",
+				Tag:            "b",
 				Percent:        ptr.Int64(50),
 				LatestRevision: ptr.Bool(true),
 			}},
diff --git a/test/e2e_constants.go b/test/e2e_constants.go
index 660de776e..4766ce9fd 100644
--- a/test/e2e_constants.go
+++ b/test/e2e_constants.go
@@ -31,6 +31,9 @@ const (
 	// ServingNamespaceforSecurityTesting is the namespace for security tests.
 	ServingNamespaceforSecurityTesting = "serving-tests-security"
 
+	// TLSNamespace is the namespace for serving tls tests
+	TLSNamespace = "tls"
+
 	// Environment propagation conformance test objects
 
 	// ConformanceConfigMap is the name of the configmap to propagate env variables from