Repository: knative/serving
Pull request #10393 (2020-12-17 01:58:11)

Optimize param passing a bit.
In fact there's no need to pass the `tag` to the lower calls,
it is only used to query rollouts for the tag, which
can be done in the top `makeIngressSpec` call.

/assign @tcnghia 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10393#" 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/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/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-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10393#pullrequestreview-554225190" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10393#" 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/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [tcnghia,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":[]} -->

Patch:
diff --git a/.github/workflows/knative-style.yaml b/.github/workflows/knative-style.yaml
index b38122ab6..3c7f28dde 100644
--- a/.github/workflows/knative-style.yaml
+++ b/.github/workflows/knative-style.yaml
@@ -209,54 +209,6 @@ jobs:
 
           echo '::endgroup::'
 
-      - name: Redundant Format
-        shell: bash
-        if: ${{ always() }}
-        env:
-          REVIEWDOG_GITHUB_API_TOKEN: ${{ github.token }}
-        run: |
-          set -e
-          cd "${GITHUB_WORKSPACE}" || exit 1
-
-          echo '::group:: Flagging possibly unnecessary format calls for trailing argument with reviewdog 🐶 ...'
-          # Don't fail because of misspell
-          set +o pipefail
-          # For all look for formatting calls and extract the ones that
-          # have only a single formatting argument and that argument is in the last position.
-          # But exclude the `fmt.Errorf` calls since there's no `fmt.Error` and `errors.New` is not
-          # as versatile.
-          # Also ignore `%T` — to print the type and `%q` to quote the final string.
-          fn=$(find . -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
-          grep -nE '[^.]+\.(Fatalf|Errorf|Warnf|Infof|Debugf|Logf|Sprintf|Fprintf|Printf)[^\%]+%[^Tqx]",[^,]+' $fn | grep -v "fmt.Errorf" |
-            while read -r ent ; do
-              file=$(echo $ent | cut -d':' -f 1);
-              line=$(echo $ent | cut -d':' -f 2);
-              ch=$(echo $ent | cut -d':' -f3-);
-              err="Unknown printer tool, please file an issue in knative-sandbox/.github and assign to @vagababov: $ch"
-              if echo $ch | grep --quiet -E "^t.(Errorf|Fatalf|Logf)" ; then
-                err=$(echo $ch | sed  -E 's/(^t\.)(Fatal|Error|Log)f([^\%]+)( %[^Tqx]",)([^,]+)/\1\2\3",\5/')
-                # Not a test. Here we deal with various loggers and fmt helpers.
-              elif echo $ch | grep --quiet "log" ; then
-                # Capture (x)?log(er)?.
-                err=$(echo $ch | sed -E 's/(.*log.*\.)(Print|Fatal|Error|Info|Warn)f([^\%]+)(%[^Tq]",)([^,]+)/\1\2\3",\5/')
-              elif echo $ch | grep --quiet -E "fmt\.Sprintf" ; then
-                # Always space after sprintf
-                err=$(echo $ch | sed -E 's/(fmt\.)(Sprint)f([^%]+) (%s",)([^,]+)/\1\2\3 ",\5/')
-              elif echo $ch | grep --quiet -E "fmt\." ; then  # all other fmt. printers.
-                err=$(echo $ch | sed -E 's/(fmt\.)(Print|Fprint)f([^%]+) (%[^sTxq]",)([^,]+)/\1\2\3",\5/')
-              fi
-              # Trim spaces.
-              ch=$(echo $ch | xargs)
-              echo "$file:$line: Please consider avoiding tail format like this:%0A- $ch%0A+ $err"
-          done |
-          reviewdog -efm="%f:%l: %m" \
-                -name="Redundant Format" \
-                -reporter="github-pr-check" \
-                -filter-mode="added" \
-                -fail-on-error="true" \
-                -level="error"
-          echo '::endgroup::'
-
       # This is mostly copied from https://github.com/get-woke/woke-action-reviewdog/blob/main/entrypoint.sh
       # since their action is not yet released under a stable version.
       - name: Language
diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
index a2fbfff4c..16dfd2866 100644
--- a/pkg/reconciler/route/resources/ingress.go
+++ b/pkg/reconciler/route/resources/ingress.go
@@ -85,8 +85,6 @@ func MakeIngressWithRollout(
 	if err != nil {
 		return nil, err
 	}
-	logger := logging.FromContext(ctx)
-	logger.Infof("@@@ Applying rollout %#v\n", ro)
 	return &netv1alpha1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      names.Ingress(r),
@@ -152,8 +150,8 @@ func makeIngressSpec(
 			if err != nil {
 				return netv1alpha1.IngressSpec{}, err
 			}
-			rule := makeIngressRule(domains, r.Namespace, name,
-				visibility, tc.Targets[name], ro)
+			rule := makeIngressRule(domains, r.Namespace,
+				visibility, tc.Targets[name], ro.RolloutsByTag(name))
 			if featuresConfig.TagHeaderBasedRouting == apicfg.Enabled {
 				if rule.HTTP.Paths[0].AppendHeaders == nil {
 					rule.HTTP.Paths[0].AppendHeaders = make(map[string]string, 1)
@@ -256,16 +254,16 @@ func makeACMEIngressPaths(challenges map[string]netv1alpha1.HTTP01Challenge, dom
 	return paths
 }
 
-func makeIngressRule(domains []string, ns, tag string,
+func makeIngressRule(domains []string, ns string,
 	visibility netv1alpha1.IngressVisibility,
 	targets traffic.RevisionTargets,
-	ro *traffic.Rollout) netv1alpha1.IngressRule {
+	roCfgs []*traffic.ConfigurationRollout) netv1alpha1.IngressRule {
 	return netv1alpha1.IngressRule{
 		Hosts:      domains,
 		Visibility: visibility,
 		HTTP: &netv1alpha1.HTTPIngressRuleValue{
 			Paths: []netv1alpha1.HTTPIngressPath{
-				*makeBaseIngressPath(ns, tag, targets, ro),
+				*makeBaseIngressPath(ns, targets, roCfgs),
 			},
 		},
 	}
@@ -276,7 +274,7 @@ func makeTagBasedRoutingIngressPaths(ns string, tc *traffic.Config, ro *traffic.
 	paths := make([]netv1alpha1.HTTPIngressPath, 0, len(names))
 
 	for _, name := range names {
-		path := makeBaseIngressPath(ns, name, tc.Targets[name], ro)
+		path := makeBaseIngressPath(ns, tc.Targets[name], ro.RolloutsByTag(name))
 		path.Headers = map[string]netv1alpha1.HeaderMatch{network.TagHeaderName: {Exact: name}}
 		paths = append(paths, *path)
 	}
@@ -294,10 +292,8 @@ func rolloutConfig(cfgName string, ros []*traffic.ConfigurationRollout) *traffic
 	return nil
 }
 
-func makeBaseIngressPath(ns, tag string, targets traffic.RevisionTargets,
-	ro *traffic.Rollout) *netv1alpha1.HTTPIngressPath {
-	roCfgs := ro.RolloutsByTag(tag)
-
+func makeBaseIngressPath(ns string, targets traffic.RevisionTargets,
+	roCfgs []*traffic.ConfigurationRollout) *netv1alpha1.HTTPIngressPath {
 	// Optimistically allocate |targets| elements.
 	splits := make([]netv1alpha1.IngressBackendSplit, 0, len(targets))
 	for _, t := range targets {
diff --git a/support/COMMUNITY_CONTACTS.md b/support/COMMUNITY_CONTACTS.md
index 14b28cdd4..498bfd7ec 100644
--- a/support/COMMUNITY_CONTACTS.md
+++ b/support/COMMUNITY_CONTACTS.md
@@ -66,21 +66,21 @@ rotation, feel free to send a PR to remove yourself.
 
 # Schedule
 
-| week       | contact                                        |
-| ---------- | ---------------------------------------------- |
-| 2020-12-14 | [tcnghia](https://github.com/tcnghia)          |
-| 2020-12-21 | (looking for a volunteer)                      |
-| 2020-12-28 | [vagababov](https://github.com/vagababov)      |
-| 2021-01-04 | [mattmoor](https://github.com/mattmoor)        |
-| 2020-01-11 | [yanweiguo](https://github.com/ZhiminXiang)    |
-| 2020-01-18 | [markusthoemmes](https://github.com/yanweiguo) |
-| 2020-01-25 | [julz](https://github.com/markusthoemmes)      |
-| 2020-02-01 | [ZhiminXiang](https://github.com/julz)         |
-| 2020-02-08 | [JRBANCEL](https://github.com/JRBANCEL)        |
-| 2020-02-15 | [nak3](https://github.com/nak3)                |
-| 2020-02-22 | [vagababov](https://github.com/vagababov)      |
-| 2020-03-01 | [tcnghia](https://github.com/tcnghia)          |
-| 2020-03-08 | [dprotaso](https://github.com/dprotaso)        |
-| 2020-03-15 | [mattmoor](https://github.com/mattmoor)        |
-| 2020-03-22 | [ZhiminXiang](https://github.com/ZhiminXiang)  |
-| 2020-03-29 | [yanweiguo](https://github.com/yanweiguo)      |
+| week       | contact                                             |
+| ---------- | --------------------------------------------------- |
+| 2020-12-14 | [tcnghia](https://github.com/tcnghia)               |
+| 2020-12-21 | [nak3](https://github.com/nak3)                     |
+| 2020-12-28 | [vagababov](https://github.com/vagababov)           |
+| 2021-01-04 | [mattmoor](https://github.com/mattmoor)             |
+| 2020-01-11 | [yanweiguo](https://github.com/yanweiguo)           |
+| 2020-01-18 | [markusthoemmes](https://github.com/markusthoemmes) |
+| 2020-01-25 | [julz](https://github.com/julz)                     |
+| 2020-02-01 | [ZhiminXiang](https://github.com/ZhiminXiang)       |
+| 2020-02-08 | [JRBANCEL](https://github.com/JRBANCEL)             |
+| 2020-02-15 | [nak3](https://github.com/nak3)                     |
+| 2020-02-22 | [vagababov](https://github.com/vagababov)           |
+| 2020-03-01 | [tcnghia](https://github.com/tcnghia)               |
+| 2020-03-08 | [dprotaso](https://github.com/dprotaso)             |
+| 2020-03-15 | [mattmoor](https://github.com/mattmoor)             |
+| 2020-03-22 | [ZhiminXiang](https://github.com/ZhiminXiang)       |
+| 2020-03-29 | [yanweiguo](https://github.com/yanweiguo)           |


Test patch:
diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go
index 3a6d56537..7fc2b840a 100644
--- a/pkg/reconciler/route/resources/ingress_test.go
+++ b/pkg/reconciler/route/resources/ingress_test.go
@@ -880,8 +880,8 @@ func TestMakeIngressRuleVanilla(t *testing.T) {
 		},
 	}
 	ro := tc.BuildRollout()
-	rule := makeIngressRule(domains, ns, traffic.DefaultTarget,
-		netv1alpha1.IngressVisibilityExternalIP, targets, ro)
+	rule := makeIngressRule(domains, ns,
+		netv1alpha1.IngressVisibilityExternalIP, targets, ro.RolloutsByTag(traffic.DefaultTarget))
 	expected := netv1alpha1.IngressRule{
 		Hosts: []string{
 			"a.com",
@@ -934,8 +934,8 @@ func TestMakeIngressRuleZeroPercentTarget(t *testing.T) {
 		},
 	}
 	ro := tc.BuildRollout()
-	rule := makeIngressRule(domains, ns, traffic.DefaultTarget,
-		netv1alpha1.IngressVisibilityExternalIP, targets, ro)
+	rule := makeIngressRule(domains, ns,
+		netv1alpha1.IngressVisibilityExternalIP, targets, ro.RolloutsByTag(traffic.DefaultTarget))
 	expected := netv1alpha1.IngressRule{
 		Hosts: []string{"test.org"},
 		HTTP: &netv1alpha1.HTTPIngressRuleValue{
@@ -986,7 +986,8 @@ func TestMakeIngressRuleTwoTargets(t *testing.T) {
 	}
 	ro := tc.BuildRollout()
 	domains := []string{"test.org"}
-	rule := makeIngressRule(domains, ns, "a-tag", netv1alpha1.IngressVisibilityExternalIP, targets, ro)
+	rule := makeIngressRule(domains, ns, netv1alpha1.IngressVisibilityExternalIP,
+		targets, ro.RolloutsByTag("a-tag"))
 	expected := netv1alpha1.IngressRule{
 		Hosts: []string{"test.org"},
 		HTTP: &netv1alpha1.HTTPIngressRuleValue{