Repository: knative/pkg
Pull request #2549 (2022-07-18 16:01:22)

Drop error from changeset.Get signature
We simply return 'unknown' if we can't read the build info. We already return 'unknown' if the vcs.revision key is missing from the build settings

Discussion: https://github.com/knative/pkg/pull/2548#discussion_r923510432

/assign @imjasonh 


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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2549#" 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":[]} -->
knative-prow[bot]: @dprotaso: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_pkg_main | b6971db5c5530a83f023b16ffe7f26a29771f9f4 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2549/unit-tests_pkg_main/1549061783740747776) | true | `/test unit-tests_pkg_main`

[Your PR dashboard](https://prow.knative.dev/pr).

<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 -->
knative-prow[bot]: New changes are detected. LGTM label has been removed.
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2549#" title="Author self-approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2549#pullrequestreview-1042139941" title="Approved">imjasonh</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/changeset/commit.go b/changeset/commit.go
index b4dcab002..93167c940 100644
--- a/changeset/commit.go
+++ b/changeset/commit.go
@@ -17,7 +17,6 @@ limitations under the License.
 package changeset
 
 import (
-	"errors"
 	"regexp"
 	"runtime/debug"
 	"strconv"
@@ -32,26 +31,25 @@ var (
 	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
+// If there is no embedded information 'unknown' will be returned
 //
 // The result will have a '-dirty' suffix if the workspace was not clean
-func Get() (string, error) {
+func Get() string {
 	once.Do(func() {
-		rev, err = get()
+		rev = get()
 	})
 
-	return rev, err
+	return rev
 }
 
-func get() (string, error) {
+func get() string {
 	info, ok := readBuildInfo()
 	if !ok {
-		return "", errors.New("unable to read build info")
+		return Unknown
 	}
 
 	var revision string
@@ -67,7 +65,7 @@ func get() (string, error) {
 	}
 
 	if revision == "" {
-		return Unknown, nil
+		return Unknown
 	}
 
 	if shaRegexp.MatchString(revision) {
@@ -78,5 +76,5 @@ func get() (string, error) {
 		revision += "-dirty"
 	}
 
-	return revision, nil
+	return revision
 }
diff --git a/logging/config.go b/logging/config.go
index b0fd30e07..a95dfe8fa 100644
--- a/logging/config.go
+++ b/logging/config.go
@@ -75,15 +75,14 @@ func NewLogger(configJSON string, levelOverride string, opts ...zap.Option) (*za
 }
 
 func enrichLoggerWithCommitID(logger *zap.Logger) *zap.SugaredLogger {
-	revision, err := changeset.Get()
-	if err != nil || revision == changeset.Unknown {
-		logger.Info("Unable to read vcs.revision from binary", zap.Error(err))
+	revision := changeset.Get()
+	if revision == changeset.Unknown {
+		logger.Info("Unable to read vcs.revision from binary")
 		return logger.Sugar()
 	}
 
 	// Enrich logs with the components git revision.
 	return logger.With(zap.String(logkey.Commit, revision)).Sugar()
-
 }
 
 // NewLoggerFromConfig creates a logger using the provided Config


Test patch:
diff --git a/changeset/commit_test.go b/changeset/commit_test.go
index c73bc58b1..8774dfed8 100644
--- a/changeset/commit_test.go
+++ b/changeset/commit_test.go
@@ -27,15 +27,14 @@ import (
 func TestGet(t *testing.T) {
 
 	cases := []struct {
-		name    string
-		info    *debug.BuildInfo
-		ok      bool
-		result  string
-		wantErr string
+		name   string
+		info   *debug.BuildInfo
+		ok     bool
+		result string
 	}{{
-		name:    "info fails",
-		ok:      false,
-		wantErr: "unable to read build info",
+		name:   "info fails",
+		ok:     false,
+		result: Unknown,
 	}, {
 		name:   "missing revision",
 		ok:     true,
@@ -79,18 +78,10 @@ func TestGet(t *testing.T) {
 				return c.info, c.ok
 			}
 
-			val, err := Get()
-			if c.wantErr == "" && err != nil {
-				t.Fatal("unexpected error", err)
-			} else if c.wantErr != "" && err != nil {
-				if diff := cmp.Diff(c.wantErr, err.Error()); 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)
-			}
+			got := Get()
+			want := c.result
 
-			if diff := cmp.Diff(c.result, val); diff != "" {
+			if diff := cmp.Diff(want, got); diff != "" {
 				t.Errorf("result doesn't match expected: %s", diff)
 			}
 		})
diff --git a/test/mako/sidecar.go b/test/mako/sidecar.go
index 2b6057c1e..64b1d96ca 100644
--- a/test/mako/sidecar.go
+++ b/test/mako/sidecar.go
@@ -91,8 +91,8 @@ func EscapeTag(tag string) string {
 func SetupHelper(ctx context.Context, benchmarkKey *string, benchmarkName *string, extraTags ...string) (*Client, error) {
 	tags := append(config.MustGetTags(), extraTags...)
 	// Get the commit of the benchmarks
-	commitID, err := changeset.Get()
-	if err != nil {
+	commitID := changeset.Get()
+	if commitID == changeset.Unknown {
 		log.Println("Cannot find commit ID")
 	}