Address PR feedback

- Use a sync.Once to prevent double parsing
- Support SHA256
- Allow non-SHA revision strings

changeset/commit.go

@@ -18,20 +18,35 @@ package changeset
 
 import (
 	"errors"
-	"fmt"
 	"regexp"
 	"runtime/debug"
 	"strconv"
+	"sync"
 )
 
-var commitIDRE = regexp.MustCompile(`^[a-f0-9]{40}$`)
-var readBuildInfo = debug.ReadBuildInfo
+var (
+	shaRegexp = regexp.MustCompile(`^[a-f0-9]{40,64}$`)
+	rev       string
+	err       error
+	once      sync.Once
+
+	// for testing
+	readBuildInfo = debug.ReadBuildInfo
+)
 
 // Get returns the 'vcs.revision' property from the embedded build information
 // This function will return an error if value is not a valid Git SHA
 //
 // The result will have a '-dirty' suffix if the workspace was not clean
 func Get() (string, error) {
+	once.Do(func() {
+		rev, err = get()
+	})
+
+	return rev, err
+}
+
+func get() (string, error) {
 	info, ok := readBuildInfo()
 	if !ok {
 		return "", errors.New("unable to read build info")
@@ -49,13 +64,17 @@ func Get() (string, error) {
 		}
 	}
 
-	if !commitIDRE.MatchString(revision) {
-		return "", fmt.Errorf("%q is not a valid git sha", revision)
+	if revision == "" {
+		return "unknown", nil
+	}
+
+	if shaRegexp.MatchString(revision) {
+		revision = revision[:7]
 	}
 
 	if modified {
-		return revision[:7] + "-dirty", nil
+		revision += "-dirty"
 	}
 
-	return revision[:7], nil
+	return revision, nil
 }

changeset/commit_test.go

@@ -18,6 +18,7 @@ package changeset
 
 import (
 	"runtime/debug"
+	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -36,30 +37,30 @@ func TestGet(t *testing.T) {
 		ok:      false,
 		wantErr: "unable to read build info",
 	}, {
-		name:    "missing revision",
-		ok:      true,
-		info:    &debug.BuildInfo{},
-		wantErr: `"" is not a valid git sha`,
+		name:   "missing revision",
+		ok:     true,
+		info:   &debug.BuildInfo{},
+		result: "unknown",
 	}, {
-		name: "bad revision",
+		name: "SHA1 revision is truncated",
 		ok:   true,
 		info: &debug.BuildInfo{
 			Settings: []debug.BuildSetting{{
-				Key: "vcs.revision", Value: "bad",
+				Key: "vcs.revision", Value: "3666ce749d32abe7be0528380c8c05a4282cb733",
 			}},
 		},
-		wantErr: `"bad" is not a valid git sha`,
+		result: "3666ce7",
 	}, {
-		name: "happy revision",
+		name: "SHA256 revision is truncated",
 		ok:   true,
 		info: &debug.BuildInfo{
 			Settings: []debug.BuildSetting{{
-				Key: "vcs.revision", Value: "3666ce749d32abe7be0528380c8c05a4282cb733",
+				Key: "vcs.revision", Value: "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824",
 			}},
 		},
-		result: "3666ce7",
+		result: "2cf24db",
 	}, {
-		name: "dirty revision",
+		name: "modified workspace results in -dirty suffix",
 		ok:   true,
 		info: &debug.BuildInfo{
 			Settings: []debug.BuildSetting{{
@@ -73,17 +74,20 @@ func TestGet(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
+			once = sync.Once{}
 			readBuildInfo = func() (info *debug.BuildInfo, ok bool) {
 				return c.info, c.ok
 			}
 
 			val, err := Get()
 			if c.wantErr == "" && err != nil {
 				t.Fatal("unexpected error", err)
-			} else if c.wantErr != "" {
+			} else if c.wantErr != "" && err != nil {
 				if diff := cmp.Diff(c.wantErr, err.Error()); diff != "" {
-					t.Errorf("error doesn't match expected: %s", diff)
+					t.Fatalf("error doesn't match expected: %s", diff)
 				}
+			} else if c.wantErr != "" && err == nil {
+				t.Fatalf("expected error %q but was nil", c.wantErr)
 			}
 
 			if diff := cmp.Diff(c.result, val); diff != "" {

logging/config.go

@@ -82,7 +82,7 @@ func enrichLoggerWithCommitID(logger *zap.Logger) *zap.SugaredLogger {
 	}
 
 	// Enrich logs with the components git revision.
-	return logger.With(zap.String(logkey.GitRevision, revision)).Sugar()
+	return logger.With(zap.String(logkey.Commit, revision)).Sugar()
 
 }
 

logging/logkey/constants.go

@@ -59,7 +59,7 @@ const (
 	// KubernetesService is the key used to represent a Kubernetes service name in logs
 	KubernetesService = "knative.dev/k8sservice"
 
-	// GitRevision is the logging key used to represent the git revision that the
+	// Commit is the logging key used to represent the VCS revision that the
 	// Knative component was built from
-	GitRevision = "commit"
+	Commit = "commit"
 )