Repository: knative/pkg
Pull request #2460 (2022-03-11 10:46:46)

Take pointer to map in AsOptionalMap for consistency
- Other `As*` functions take pointers.
- This avoids having to initialize the map passed to the function

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2460#" title="Author self-approved">pierDipi</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **julz** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @julz` 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%2Fpkg).

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

- **[OWNERS](https://github.com/knative/pkg/blob/main/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":["julz"]} -->
pierDipi: @nak3 with this we don't have to update serving or networking.

/cc @dprotaso @nak3 
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2460?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2460](https://codecov.io/gh/knative/pkg/pull/2460?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (8dd9082) into [main](https://codecov.io/gh/knative/pkg/commit/ce6f2877cef94d5c91bafe2ce4acba52ee22c1fe?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (ce6f287) will **decrease** coverage by `0.08%`.
> The diff coverage is `88.23%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2460/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/2460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #2460      +/-   ##
==========================================
- Coverage   63.61%   63.53%   -0.09%     
==========================================
  Files         228      228              
  Lines       10016    10003      -13     
==========================================
- Hits         6372     6355      -17     
- Misses       3342     3344       +2     
- Partials      302      304       +2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [leaderelection/config.go](https://codecov.io/gh/knative/pkg/pull/2460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-bGVhZGVyZWxlY3Rpb24vY29uZmlnLmdv) | `76.00% <84.61%> (-4.00%)` | :arrow_down: |
| [configmap/parse.go](https://codecov.io/gh/knative/pkg/pull/2460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-Y29uZmlnbWFwL3BhcnNlLmdv) | `96.22% <100.00%> (+0.10%)` | :arrow_up: |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/2460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `90.32% <0.00%> (-1.08%)` | :arrow_down: |
| [hash/bucketer.go](https://codecov.io/gh/knative/pkg/pull/2460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-aGFzaC9idWNrZXRlci5nbw==) | `100.00% <0.00%> (ø)` | |
| [reconciler/retry.go](https://codecov.io/gh/knative/pkg/pull/2460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cmVjb25jaWxlci9yZXRyeS5nbw==) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2460?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/2460?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [ce6f287...8dd9082](https://codecov.io/gh/knative/pkg/pull/2460?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: /lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2460#issuecomment-1067049300" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2460#" title="Author self-approved">pierDipi</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":[]} -->

Patch:
diff --git a/configmap/parse.go b/configmap/parse.go
index b2f8d4f20..e733e3838 100644
--- a/configmap/parse.go
+++ b/configmap/parse.go
@@ -245,13 +245,21 @@ func Parse(data map[string]string, parsers ...ParseFunc) error {
 	return nil
 }
 
-// AsOptionalMap parses the data into the target as a map[string]string, if it exists.
+// CollectMapEntriesWithPrefix parses the data into the target as a map[string]string, if it exists.
 // The map is represented as a list of key-value pairs with a common prefix.
-func AsOptionalMap(prefix string, target map[string]string) ParseFunc {
+func CollectMapEntriesWithPrefix(prefix string, target *map[string]string) ParseFunc {
+	if target == nil {
+		panic("target cannot be nil")
+	}
+
 	return func(data map[string]string) error {
 		for k, v := range data {
 			if strings.HasPrefix(k, prefix) && len(k) > len(prefix)+1 {
-				target[k[len(prefix)+1: /* remove dot `.` */]] = v
+				if *target == nil {
+					m := make(map[string]string, 2)
+					*target = m
+				}
+				(*target)[k[len(prefix)+1: /* remove dot `.` */]] = v
 			}
 		}
 		return nil
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 13e0832e5..11010014a 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -57,7 +57,7 @@ func NewConfigFromMap(data map[string]string) (*Config, error) {
 
 		cm.AsUint32("buckets", &config.Buckets),
 
-		cm.AsOptionalMap("map-lease-prefix", config.LeaseNamesPrefixMapping),
+		cm.CollectMapEntriesWithPrefix("map-lease-prefix", &config.LeaseNamesPrefixMapping),
 	); err != nil {
 		return nil, err
 	}
@@ -101,11 +101,10 @@ func (c *Config) GetComponentConfig(name string) ComponentConfig {
 
 func defaultConfig() *Config {
 	return &Config{
-		Buckets:                 1,
-		LeaseDuration:           60 * time.Second,
-		RenewDeadline:           40 * time.Second,
-		RetryPeriod:             10 * time.Second,
-		LeaseNamesPrefixMapping: make(map[string]string),
+		Buckets:       1,
+		LeaseDuration: 60 * time.Second,
+		RenewDeadline: 40 * time.Second,
+		RetryPeriod:   10 * time.Second,
 	}
 }
 


Test patch:
diff --git a/configmap/parse_test.go b/configmap/parse_test.go
index 725b11bd2..b948b3f01 100644
--- a/configmap/parse_test.go
+++ b/configmap/parse_test.go
@@ -76,9 +76,6 @@ func TestParse(t *testing.T) {
 			"test-dict.k":  "v",
 			"test-dict.k1": "v1",
 		},
-		conf: testConfig{
-			dict: map[string]string{},
-		},
 		want: testConfig{
 			str:    "foo.bar",
 			toggle: true,
@@ -218,13 +215,13 @@ func TestParse(t *testing.T) {
 				AsQuantity("test-quantity", &test.conf.qua),
 				AsNamespacedName("test-namespaced-name", &test.conf.nsn),
 				AsOptionalNamespacedName("test-optional-namespaced-name", &test.conf.onsn),
-				AsOptionalMap("test-dict", test.conf.dict),
+				CollectMapEntriesWithPrefix("test-dict", &test.conf.dict),
 			); (err == nil) == test.expectErr {
 				t.Fatal("Failed to parse data:", err)
 			}
 
-			if !cmp.Equal(test.conf, test.want, cmp.AllowUnexported(testConfig{})) {
-				t.Fatalf("parsed = %v, want %v", test.conf, test.want)
+			if diff := cmp.Diff(test.want, test.conf, cmp.AllowUnexported(testConfig{})); diff != "" {
+				t.Fatal("(-want, +got)", diff)
 			}
 		})
 	}
diff --git a/leaderelection/config_test.go b/leaderelection/config_test.go
index 6e821831e..fcb699eb8 100644
--- a/leaderelection/config_test.go
+++ b/leaderelection/config_test.go
@@ -38,11 +38,10 @@ const (
 
 func okConfig() *Config {
 	return &Config{
-		Buckets:                 1,
-		LeaseDuration:           15 * time.Second,
-		RenewDeadline:           10 * time.Second,
-		RetryPeriod:             2 * time.Second,
-		LeaseNamesPrefixMapping: map[string]string{},
+		Buckets:       1,
+		LeaseDuration: 15 * time.Second,
+		RenewDeadline: 10 * time.Second,
+		RetryPeriod:   2 * time.Second,
 	}
 }
 
@@ -123,11 +122,10 @@ func TestNewConfigMapFromData(t *testing.T) {
 			"buckets":       "5",
 		},
 		expected: &Config{
-			Buckets:                 5,
-			LeaseDuration:           2 * time.Second,
-			RenewDeadline:           3 * time.Second,
-			RetryPeriod:             4 * time.Second,
-			LeaseNamesPrefixMapping: map[string]string{},
+			Buckets:       5,
+			LeaseDuration: 2 * time.Second,
+			RenewDeadline: 3 * time.Second,
+			RetryPeriod:   4 * time.Second,
 		},
 	}, {
 		name: "prioritize new keys",
@@ -141,11 +139,10 @@ func TestNewConfigMapFromData(t *testing.T) {
 			"buckets":        "7",
 		},
 		expected: &Config{
-			Buckets:                 7,
-			LeaseDuration:           1 * time.Second,
-			RenewDeadline:           2 * time.Second,
-			RetryPeriod:             3 * time.Second,
-			LeaseNamesPrefixMapping: map[string]string{},
+			Buckets:       7,
+			LeaseDuration: 1 * time.Second,
+			RenewDeadline: 2 * time.Second,
+			RetryPeriod:   3 * time.Second,
 		},
 	}}
 
@@ -185,11 +182,10 @@ func TestNewConfigFromMap(t *testing.T) {
 			"buckets":        "5",
 		},
 		want: Config{
-			Buckets:                 5,
-			LeaseDuration:           15 * time.Second,
-			RenewDeadline:           40 * time.Second,
-			RetryPeriod:             10 * time.Second,
-			LeaseNamesPrefixMapping: map[string]string{},
+			Buckets:       5,
+			LeaseDuration: 15 * time.Second,
+			RenewDeadline: 40 * time.Second,
+			RetryPeriod:   10 * time.Second,
 		},
 	}, {
 		name: "ok config, prefix map",