Repository: knative/pkg
Pull request #2355 (2021-11-19 22:37:11)

kmap changes
- add list variant to exclude keys
- Rename OrderedLookup to be KeyPriority


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

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

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

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/pkg/blob/main/OWNERS)~~ [dprotaso]

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":[]} -->
dprotaso: /assign @mattmoor 
/hold just to check the ergonomics of using the API - KeyPriority is now a type alias of a string array
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2355#" title="Author self-approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2355#pullrequestreview-811734169" title="Approved">mattmoor</a>*

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

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/pkg/blob/main/OWNERS)~~ [dprotaso,mattmoor]

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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2355](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (d0cfd6e) into [main](https://codecov.io/gh/knative/pkg/commit/a99300deff34c04163d69ef7e55ae7c2a87fe5da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (a99300d) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2355/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #2355      +/-   ##
==========================================
+ Coverage   63.58%   63.62%   +0.04%     
==========================================
  Files         228      228              
  Lines        9851     9854       +3     
==========================================
+ Hits         6264     6270       +6     
+ Misses       3310     3307       -3     
  Partials      277      277              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [kmap/lookup.go](https://codecov.io/gh/knative/pkg/pull/2355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-a21hcC9sb29rdXAuZ28=) | `100.00% <100.00%> (+25.00%)` | :arrow_up: |
| [kmap/map.go](https://codecov.io/gh/knative/pkg/pull/2355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-a21hcC9tYXAuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [a99300d...d0cfd6e](https://codecov.io/gh/knative/pkg/pull/2355?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

dprotaso: /hold cancel

Patch:
diff --git a/kmap/lookup.go b/kmap/lookup.go
index 6066c00b6..e24041b6e 100644
--- a/kmap/lookup.go
+++ b/kmap/lookup.go
@@ -16,25 +16,24 @@ limitations under the License.
 
 package kmap
 
-// OrderedLookup is a utility struct for getting values from a map
+// KeyPriority is a utility struct for getting values from a map
 // given a list of ordered keys
 //
 // This is to help the migration/renaming of annotations & labels
-type OrderedLookup struct {
-	Keys []string
-}
+type KeyPriority []string
 
 // Key returns the default key that should be used for
 // accessing the map
-func (a *OrderedLookup) Key() string {
-	return a.Keys[0]
+func (p KeyPriority) Key() string {
+	// this intentionally panics rather than returning an empty string
+	return p[0]
 }
 
 // Value iterates looks up the ordered keys in the map and returns
 // a string value. An empty string will be returned if the keys
 // are not present in the map
-func (a *OrderedLookup) Value(m map[string]string) string {
-	_, v, _ := a.Get(m)
+func (p KeyPriority) Value(m map[string]string) string {
+	_, v, _ := p.Get(m)
 	return v
 }
 
@@ -46,26 +45,33 @@ func (a *OrderedLookup) Value(m map[string]string) string {
 //
 // If no key is present the default key (lowest ordinal) is returned
 // with an empty string as the value
-func (a *OrderedLookup) Get(m map[string]string) (string, string, bool) {
+func (p KeyPriority) Get(m map[string]string) (string, string, bool) {
 	var k, v string
 	var ok bool
-	for _, k = range a.Keys {
+	for _, k = range p {
 		v, ok = m[k]
 		if ok {
 			return k, v, ok
 		}
 	}
 
-	return a.Keys[0], "", false
+	return p.Key(), "", false
 }
 
-// NewOrderedLookup builds a utilty struct for looking up N keys
-// in a map in a specific order
-//
-// If no keys are supplied this method will panic
-func NewOrderedLookup(keys ...string) *OrderedLookup {
-	if len(keys) == 0 {
-		panic("expected to have at least a single key")
+// UpdateKey will update the map with the KeyPriority's default
+// key iff any of the other synonym keys are present
+func (p KeyPriority) UpdateKey(m map[string]string) {
+	if k, v, ok := p.Get(m); ok && k != p.Key() {
+		delete(m, k)
+		m[p.Key()] = v
+	}
+}
+
+// UpdateKeys iterates over the lookups and updates entries in the map
+// to use the default key
+func UpdateKeys(m map[string]string, keys ...KeyPriority) map[string]string {
+	for _, key := range keys {
+		key.UpdateKey(m)
 	}
-	return &OrderedLookup{Keys: keys}
+	return m
 }
diff --git a/kmap/map.go b/kmap/map.go
index 98a521ee7..006348d76 100644
--- a/kmap/map.go
+++ b/kmap/map.go
@@ -57,6 +57,11 @@ func Filter(in map[string]string, filter func(string) bool) map[string]string {
 
 // ExcludeKeys creates a copy of the provided map filtering out the excluded `keys`
 func ExcludeKeys(in map[string]string, keys ...string) map[string]string {
+	return ExcludeKeyList(in, keys)
+}
+
+// ExcludeKeyList creates a copy of the provided map filtering out excluded `keys`
+func ExcludeKeyList(in map[string]string, keys []string) map[string]string {
 	ret := make(map[string]string, len(in))
 
 outer:


Test patch:
diff --git a/kmap/lookup_test.go b/kmap/lookup_test.go
index 8bd24162b..0018ff94f 100644
--- a/kmap/lookup_test.go
+++ b/kmap/lookup_test.go
@@ -22,49 +22,31 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-func TestNewOrderedLookup(t *testing.T) {
-	keys := []string{"1", "2", "3"}
-	a := NewOrderedLookup(keys...)
-
-	if diff := cmp.Diff(keys, a.Keys); diff != "" {
-		t.Error("NewOrderedLookup unexpected diff (-want, +got):", diff)
-	}
-}
-
-func TestNewOrderedLookup_BadInput(t *testing.T) {
-	defer func() {
-		if r := recover(); r == nil {
-			t.Error("expected no keys to panic")
-		}
-	}()
-	NewOrderedLookup()
-}
-
-func TestOrderedLookup_Get(t *testing.T) {
+func TestKeyPriority(t *testing.T) {
 	tests := []struct {
 		name     string
-		keys     []string
+		keys     KeyPriority
 		in       map[string]string
 		outKey   string
 		outValue string
 		outOk    bool
 	}{{
 		name:     "single key in map",
-		keys:     []string{"old", "new"},
+		keys:     KeyPriority{"old", "new"},
 		in:       map[string]string{"old": "1"},
 		outKey:   "old",
 		outValue: "1",
 		outOk:    true,
 	}, {
 		name:     "another single key in map",
-		keys:     []string{"old", "new"},
+		keys:     KeyPriority{"old", "new"},
 		in:       map[string]string{"new": "1"},
 		outKey:   "new",
 		outValue: "1",
 		outOk:    true,
 	}, {
 		name:     "lower ordinal takes priority",
-		keys:     []string{"old", "new"},
+		keys:     KeyPriority{"old", "new"},
 		in:       map[string]string{"new": "1", "old": "2"},
 		outKey:   "old",
 		outValue: "2",
@@ -82,9 +64,7 @@ func TestOrderedLookup_Get(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			a := NewOrderedLookup(tc.keys...)
-
-			k, v, ok := a.Get(tc.in)
+			k, v, ok := tc.keys.Get(tc.in)
 
 			if tc.outOk != ok {
 				t.Error("expected ok to be", tc.outOk)
@@ -98,7 +78,54 @@ func TestOrderedLookup_Get(t *testing.T) {
 				t.Errorf("expected key to be %q got: %q", tc.outKey, k)
 			}
 
+			if want, got := v, tc.keys.Value(tc.in); got != want {
+				t.Errorf("Value() diff %q != %q", want, got)
+			}
 		})
 	}
+}
+
+func TestUpdateKeys(t *testing.T) {
+	tests := []struct {
+		name string
+		in   map[string]string
+		out  map[string]string
+		ols  []KeyPriority
+	}{{
+		name: "identity",
+		in:   map[string]string{"1": "2"},
+		out:  map[string]string{"1": "2"},
+	}, {
+		name: "single replacement",
+		in:   map[string]string{"1": "2", "4": "5"},
+		ols: []KeyPriority{
+			{"3", "1"},
+		},
+		out: map[string]string{"3": "2", "4": "5"},
+	}, {
+		name: "multiple replacement",
+		in:   map[string]string{"1": "2", "6": "5", "8": "9"},
+		ols: []KeyPriority{
+			{"3", "1"},
+			{"4", "6"},
+		},
+		out: map[string]string{"3": "2", "4": "5", "8": "9"},
+	}, {
+		name: "default not replaced",
+		in:   map[string]string{"1": "2", "4": "5"},
+		ols: []KeyPriority{
+			{"1", "3"},
+		},
+		out: map[string]string{"1": "2", "4": "5"},
+	}}
 
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+
+			out := UpdateKeys(tc.in, tc.ols...)
+			if diff := cmp.Diff(tc.out, out); diff != "" {
+				t.Error("Migrate diff (-want,+got):", diff)
+			}
+		})
+	}
 }
diff --git a/kmap/map_test.go b/kmap/map_test.go
index 81be1b80a..6701ed6ab 100644
--- a/kmap/map_test.go
+++ b/kmap/map_test.go
@@ -186,6 +186,10 @@ func TestExcludeKeys(t *testing.T) {
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Error("ExcludeKeys (-want, +got) =", diff)
 			}
+			got = ExcludeKeyList(test.in, test.exclude)
+			if diff := cmp.Diff(test.want, got); diff != "" {
+				t.Error("ExcludeKeyList (-want, +got) =", diff)
+			}
 		})
 	}
 }