Repository: knative/pkg
Pull request #1374 (2020-06-03 08:21:57)

Use a hexadecimal representation for the example checksum.
As per @julz comment on https://github.com/knative/serving/pull/8123 this is more usual. I like it better anyway as the `Checksum` function now controls the representation.

/assign @julz 

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

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

- **[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)**
- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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":["mattmoor"]} -->
julz: 👍 love it.

/lgtm
markusthoemmes: /assign @mattmoor 
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/example.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1374/pull-knative-pkg-go-coverage/1268095725892276224/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[configmap/hash-gen/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1374/pull-knative-pkg-go-coverage/1268095725892276224/artifacts/line-cov.html#file1) | Do not exist | 65.2% | 
[webhook/configmaps/configmaps.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1374/pull-knative-pkg-go-coverage/1268095725892276224/artifacts/line-cov.html#file2) | 93.9% | 94.3% | 0.3

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1374#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1374#pullrequestreview-423674470" 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:

- ~~[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)~~ [mattmoor]
- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [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":[]} -->

Patch:
diff --git a/configmap/example.go b/configmap/example.go
index ee17666b7..2ffe448e5 100644
--- a/configmap/example.go
+++ b/configmap/example.go
@@ -17,6 +17,7 @@ limitations under the License.
 package configmap
 
 import (
+	"fmt"
 	"hash/crc32"
 )
 
@@ -29,6 +30,6 @@ const (
 )
 
 // Checksum generates a checksum for the example value to be compared against a respective label.
-func Checksum(value string) uint32 {
-	return crc32.ChecksumIEEE([]byte(value))
+func Checksum(value string) string {
+	return fmt.Sprintf("%08x", crc32.ChecksumIEEE([]byte(value)))
 }
diff --git a/configmap/hash-gen/main.go b/configmap/hash-gen/main.go
index ff105b7d7..c6e5110e3 100644
--- a/configmap/hash-gen/main.go
+++ b/configmap/hash-gen/main.go
@@ -74,7 +74,7 @@ func process(data []byte) ([]byte, error) {
 		return nil, errors.New("'metadata.labels' not found")
 	}
 
-	checksum := fmt.Sprint(configmap.Checksum(example.Value))
+	checksum := configmap.Checksum(example.Value)
 	existingLabel := value(labels, configmap.ExampleChecksumLabel)
 	if existingLabel != nil {
 		existingLabel.Value = checksum
diff --git a/webhook/configmaps/configmaps.go b/webhook/configmaps/configmaps.go
index 8cc150b0a..f6ffb81b7 100644
--- a/webhook/configmaps/configmaps.go
+++ b/webhook/configmaps/configmaps.go
@@ -187,7 +187,7 @@ func (ac *reconciler) validate(ctx context.Context, req *admissionv1beta1.Admiss
 	exampleData, hasExampleData := newObj.Data[configmap.ExampleKey]
 	exampleChecksum, hasExampleChecksumLabel := newObj.Labels[configmap.ExampleChecksumLabel]
 	if hasExampleData && hasExampleChecksumLabel &&
-		exampleChecksum != fmt.Sprint(configmap.Checksum(exampleData)) {
+		exampleChecksum != configmap.Checksum(exampleData) {
 		return fmt.Errorf(
 			"%q modified, you likely wanted to create an unindented configuration",
 			configmap.ExampleKey)


Test patch:
diff --git a/configmap/example_test.go b/configmap/example_test.go
index df6734433..aafc951f0 100644
--- a/configmap/example_test.go
+++ b/configmap/example_test.go
@@ -21,21 +21,21 @@ import "testing"
 func TestChecksum(t *testing.T) {
 	tests := []struct {
 		in   string
-		want uint32
+		want string
 	}{{
 		in:   "",
-		want: 0,
+		want: "00000000",
 	}, {
 		in:   "1",
-		want: 2212294583,
+		want: "83dcefb7",
 	}, {
 		in:   "a somewhat longer test",
-		want: 4278087538,
+		want: "fefe6f72",
 	}}
 
 	for _, test := range tests {
 		if got := Checksum(test.in); got != test.want {
-			t.Errorf("Checksum(%q) = %d, want %d", test.in, got, test.want)
+			t.Errorf("Checksum(%q) = %s, want %s", test.in, got, test.want)
 		}
 	}
 }
diff --git a/configmap/hash-gen/testdata/add_want.yaml b/configmap/hash-gen/testdata/add_want.yaml
index 2cccc9b4d..22de0a376 100644
--- a/configmap/hash-gen/testdata/add_want.yaml
+++ b/configmap/hash-gen/testdata/add_want.yaml
@@ -19,7 +19,7 @@ metadata:
   namespace: knative-serving
   labels:
     serving.knative.dev/release: devel
-    knative.dev/example-checksum: "2958957236"
+    knative.dev/example-checksum: b05e1ab4
 data:
   leave-me: "alone"
   _example: |
diff --git a/configmap/hash-gen/testdata/update_want.yaml b/configmap/hash-gen/testdata/update_want.yaml
index 2cccc9b4d..22de0a376 100644
--- a/configmap/hash-gen/testdata/update_want.yaml
+++ b/configmap/hash-gen/testdata/update_want.yaml
@@ -19,7 +19,7 @@ metadata:
   namespace: knative-serving
   labels:
     serving.knative.dev/release: devel
-    knative.dev/example-checksum: "2958957236"
+    knative.dev/example-checksum: b05e1ab4
 data:
   leave-me: "alone"
   _example: |
diff --git a/configmap/testing/configmap.go b/configmap/testing/configmap.go
index bd61285bf..5c4a2d8c1 100644
--- a/configmap/testing/configmap.go
+++ b/configmap/testing/configmap.go
@@ -86,7 +86,7 @@ func ConfigMapsFromTestFile(t *testing.T, name string, allowed ...string) (*core
 	// Check that the hashed exampleBody matches the assigned label, if present.
 	gotChecksum, hasExampleChecksumLabel := orig.Labels[configmap.ExampleChecksumLabel]
 	if hasExampleBody && hasExampleChecksumLabel {
-		wantChecksum := fmt.Sprint(configmap.Checksum(exampleBody))
+		wantChecksum := configmap.Checksum(exampleBody)
 		if gotChecksum != wantChecksum {
 			t.Errorf("example checksum label = %s, want %s", gotChecksum, wantChecksum)
 		}