Repository: knative/pkg
Pull request #1600 (2020-08-10 18:36:43)

add helper to parse namespaced names

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1600#" 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:

- ~~[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/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":[]} -->
knative-prow-robot: @dprotaso: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | b9c2c9c95a0ec14725c8fc62a852fbe705765073 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1600/pull-knative-pkg-unit-tests/1292892804195815424) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1600). [Your PR dashboard](https://gubernator.knative.dev/pr/dprotaso).

<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 -->
markusthoemmes: Any reason not to use `cache.SplitMetaNamespaceKey`?
dprotaso: > cache.SplitMetaNamespaceKey

Funny enough I was looking for that and thought it was in `knative.dev/pkg`

I don't think we need to pull in that package for this logic. Though go1.15 is around the corner and has improvements to reduce the size

ok can folks PTAL again 
knative-prow-robot: @dprotaso: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 3cb713eca9b8bc1ec58430f1f35391af61dd7ae9 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1600/pull-knative-pkg-unit-tests/1292906645860388864) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1600). [Your PR dashboard](https://gubernator.knative.dev/pr/dprotaso).

<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 -->
dprotaso: alright this blew up - PTAL again

thanks for the reviews!
julz: Let's land it

/lgtm
/unhold

Patch:
diff --git a/configmap/parse.go b/configmap/parse.go
index 1ef675818..eba04a11c 100644
--- a/configmap/parse.go
+++ b/configmap/parse.go
@@ -23,6 +23,8 @@ import (
 	"time"
 
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -144,6 +146,47 @@ func AsQuantity(key string, target **resource.Quantity) ParseFunc {
 	}
 }
 
+// AsOptionalNamespacedName parses the value at key as a types.NamespacedName into the target, if it exists
+// The namespace and name are both required and expected to be valid DNS labels
+func AsOptionalNamespacedName(key string, target **types.NamespacedName) ParseFunc {
+	return func(data map[string]string) error {
+		if _, ok := data[key]; !ok {
+			return nil
+		}
+
+		*target = &types.NamespacedName{}
+		return AsNamespacedName(key, *target)(data)
+	}
+}
+
+// AsNamespacedName parses the value at key as a types.NamespacedName into the target, if it exists
+// The namespace and name are both required and expected to be valid DNS labels
+func AsNamespacedName(key string, target *types.NamespacedName) ParseFunc {
+	return func(data map[string]string) error {
+		raw, ok := data[key]
+		if !ok {
+			return nil
+		}
+
+		v := strings.SplitN(raw, string(types.Separator), 3)
+
+		if len(v) != 2 {
+			return fmt.Errorf("failed to parse %q: expected 'namespace/name' format", key)
+		}
+
+		for _, val := range v {
+			if errs := validation.ValidateNamespaceName(val, false); len(errs) > 0 {
+				return fmt.Errorf("failed to parse %q: %s", key, strings.Join(errs, ", "))
+			}
+		}
+
+		target.Namespace = v[0]
+		target.Name = v[1]
+
+		return nil
+	}
+}
+
 // Parse parses the given map using the parser functions passed in.
 func Parse(data map[string]string, parsers ...ParseFunc) error {
 	for _, parse := range parsers {


Test patch:
diff --git a/configmap/parse_test.go b/configmap/parse_test.go
index 26fb3332d..8bf6cb98e 100644
--- a/configmap/parse_test.go
+++ b/configmap/parse_test.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -35,6 +36,9 @@ type testConfig struct {
 	dur time.Duration
 	set sets.String
 	qua *resource.Quantity
+
+	nsn  types.NamespacedName
+	onsn *types.NamespacedName
 }
 
 func TestParse(t *testing.T) {
@@ -57,6 +61,9 @@ func TestParse(t *testing.T) {
 			"test-duration": "1m",
 			"test-set":      "a,b,c",
 			"test-quantity": "500m",
+
+			"test-namespaced-name":          "some-namespace/some-name",
+			"test-optional-namespaced-name": "some-other-namespace/some-other-name",
 		},
 		want: testConfig{
 			str: "foo.bar",
@@ -68,6 +75,14 @@ func TestParse(t *testing.T) {
 			dur: time.Minute,
 			set: sets.NewString("a", "b", "c"),
 			qua: &fiveHundredM,
+			nsn: types.NamespacedName{
+				Name:      "some-name",
+				Namespace: "some-namespace",
+			},
+			onsn: &types.NamespacedName{
+				Name:      "some-other-name",
+				Namespace: "some-other-namespace",
+			},
 		},
 	}, {
 		name: "respect defaults",
@@ -133,6 +148,18 @@ func TestParse(t *testing.T) {
 			"test-quantity": "foo",
 		},
 		expectErr: true,
+	}, {
+		name: "types.NamespacedName bad dns name error",
+		data: map[string]string{
+			"test-namespaced-name": "some.bad.name/blah.bad.name",
+		},
+		expectErr: true,
+	}, {
+		name: "types.NamespacedName bad segment count error",
+		data: map[string]string{
+			"test-namespaced-name": "default/resource/whut",
+		},
+		expectErr: true,
 	}}
 
 	for _, test := range tests {
@@ -147,6 +174,8 @@ func TestParse(t *testing.T) {
 				AsDuration("test-duration", &test.conf.dur),
 				AsStringSet("test-set", &test.conf.set),
 				AsQuantity("test-quantity", &test.conf.qua),
+				AsNamespacedName("test-namespaced-name", &test.conf.nsn),
+				AsOptionalNamespacedName("test-optional-namespaced-name", &test.conf.onsn),
 			); (err == nil) == test.expectErr {
 				t.Fatal("Failed to parse data:", err)
 			}