🚧 Add diagnostic logging when constructing metadata

jasonrudolph · jasonrudolph · commit 5f83f0d98ee0 · 2021-02-10T09:50:51.000-05:00
This approach feels very "brute force." I'd like to look for a way to
not have to pass the logger in so many places. 🤔
diff --git a/cmd/submit.go b/cmd/submit.go
@@ -143,7 +143,7 @@ func (s *Submit) Init(args []string, envs map[string]string) error {
 // Run packages up the test results and sends them to BuildPulse. It returns the
 // key that uniquely identifies the uploaded object.
 func (s *Submit) Run() (string, error) {
-	meta, err := metadata.NewMetadata(s.version, s.envs, s.commitResolver, time.Now)
+	meta, err := metadata.NewMetadata(s.version, s.envs, s.commitResolver, time.Now, s.diagnostics)
 	if err != nil {
 		return "", err
 	}
diff --git a/metadata/metadata.go b/metadata/metadata.go
@@ -2,7 +2,6 @@ package metadata
 
 import (
 	"fmt"
-	"os"
 	"regexp"
 	"strconv"
 	"strings"
@@ -18,11 +17,16 @@ import (
 type Metadata interface {
 	MarshalYAML() (out []byte, err error)
 
-	initEnvData(envs map[string]string, resolver CommitResolver) error
+	initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error
 	initTimestamp(now func() time.Time)
 	initVersionData(version *Version)
 }
 
+// Logger -- TODO Add docs
+type Logger interface {
+	Printf(format string, v ...interface{})
+}
+
 // AbstractMetadata provides the fields that are common across all Metadata
 // instances, regardless of the specific CI provider.
 type AbstractMetadata struct {
@@ -45,10 +49,10 @@ type AbstractMetadata struct {
 	TreeSHA           string    `yaml:":tree,omitempty"`
 }
 
-func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string) error {
+func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string, log Logger) error {
 	// Git metadata functionality is experimental. While it's experimental, detect a nil CommitResolver and allow the commit metadata fields to be uploaded with empty values.
 	if cr == nil {
-		fmt.Fprintf(os.Stderr, "[experimental] no commit resolver available; falling back to commit data from environment\n")
+		log.Printf("[experimental] no commit resolver available; falling back to commit data from environment\n")
 
 		a.CommitSHA = sha
 		return nil
@@ -57,7 +61,7 @@ func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string) error {
 	// Git metadata functionality is experimental. While it's experimental, don't let this error prevent the test-reporter from continuing normal operation. Allow the commit metadata fields to be uploaded with empty values.
 	c, err := cr.Lookup(sha)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "[experimental] git-based commit lookup unsuccessful; falling back to commit data from environment: %v\n", err)
+		log.Printf("[experimental] git-based commit lookup unsuccessful; falling back to commit data from environment: %v\n", err)
 
 		a.CommitSHA = sha
 		return nil
@@ -86,7 +90,7 @@ func (a *AbstractMetadata) initVersionData(version *Version) {
 }
 
 // NewMetadata creates a new Metadata instance from the given args.
-func NewMetadata(version *Version, envs map[string]string, resolver CommitResolver, now func() time.Time) (Metadata, error) {
+func NewMetadata(version *Version, envs map[string]string, resolver CommitResolver, now func() time.Time, log Logger) (Metadata, error) {
 	var m Metadata
 
 	switch {
@@ -106,7 +110,7 @@ func NewMetadata(version *Version, envs map[string]string, resolver CommitResolv
 		return nil, fmt.Errorf("unrecognized environment: system does not appear to be a supported CI provider (Buildkite, CircleCI, GitHub Actions, Jenkins, Semaphore, or Travis CI)")
 	}
 
-	if err := m.initEnvData(envs, resolver); err != nil {
+	if err := m.initEnvData(envs, resolver, log); err != nil {
 		return nil, err
 	}
 	m.initTimestamp(now)
@@ -142,12 +146,12 @@ type buildkiteMetadata struct {
 	BuildkiteTag                    string `env:"BUILDKITE_TAG" yaml:":buildkite_tag,omitempty"`
 }
 
-func (b *buildkiteMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (b *buildkiteMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(b, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := b.initCommitData(resolver, b.BuildkiteCommit); err != nil {
+	if err := b.initCommitData(resolver, b.BuildkiteCommit, log); err != nil {
 		return err
 	}
 
@@ -199,12 +203,12 @@ type circleMetadata struct {
 	CircleWorkflowID          string `env:"CIRCLE_WORKFLOW_ID" yaml:":circle_workflow_id"`
 }
 
-func (c *circleMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (c *circleMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(c, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := c.initCommitData(resolver, c.CircleSHA1); err != nil {
+	if err := c.initCommitData(resolver, c.CircleSHA1, log); err != nil {
 		return err
 	}
 
@@ -243,12 +247,12 @@ type githubMetadata struct {
 	GithubWorkflow  string `env:"GITHUB_WORKFLOW" yaml:":github_workflow"`
 }
 
-func (g *githubMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (g *githubMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(g, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := g.initCommitData(resolver, g.GithubSHA); err != nil {
+	if err := g.initCommitData(resolver, g.GithubSHA, log); err != nil {
 		return err
 	}
 
@@ -302,12 +306,12 @@ type jenkinsMetadata struct {
 	JenkinsWorkspace      string `env:"WORKSPACE" yaml:":jenkins_workspace"`
 }
 
-func (j *jenkinsMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (j *jenkinsMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(j, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := j.initCommitData(resolver, j.GitCommit); err != nil {
+	if err := j.initCommitData(resolver, j.GitCommit, log); err != nil {
 		return err
 	}
 
@@ -363,12 +367,12 @@ type semaphoreMetadata struct {
 	SemaphoreWorkflowNumber              uint   `env:"SEMAPHORE_WORKFLOW_NUMBER" yaml:":semaphore_workflow_number"`
 }
 
-func (s *semaphoreMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (s *semaphoreMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(s, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := s.initCommitData(resolver, s.SemaphoreGitSHA); err != nil {
+	if err := s.initCommitData(resolver, s.SemaphoreGitSHA, log); err != nil {
 		return err
 	}
 
@@ -419,12 +423,12 @@ type travisMetadata struct {
 	TravisTestResult        uint   `env:"TRAVIS_TEST_RESULT" yaml:":travis_test_result"`
 }
 
-func (t *travisMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {
+func (t *travisMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {
 	if err := env.Parse(t, env.Options{Environment: envs}); err != nil {
 		return err
 	}
 
-	if err := t.initCommitData(resolver, t.TravisCommit); err != nil {
+	if err := t.initCommitData(resolver, t.TravisCommit, log); err != nil {
 		return err
 	}
 
diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go
@@ -176,7 +176,7 @@ func TestNewMetadata(t *testing.T) {
 				})
 
 			version := &Version{Number: "v1.2.3", GoOS: "linux"}
-			meta, err := NewMetadata(version, tt.envs, commitResolverDouble, now)
+			meta, err := NewMetadata(version, tt.envs, commitResolverDouble, now, &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -187,7 +187,7 @@ func TestNewMetadata(t *testing.T) {
 }
 
 func TestNewMetadata_unsupportedProvider(t *testing.T) {
-	_, err := NewMetadata(&Version{}, map[string]string{}, newCommitResolverStub(), time.Now)
+	_, err := NewMetadata(&Version{}, map[string]string{}, newCommitResolverStub(), time.Now, &stubLogger{})
 	if assert.Error(t, err) {
 		assert.Contains(t, err.Error(), "unrecognized environment")
 	}
@@ -260,7 +260,7 @@ func TestNewMetadata_customCheckName(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			meta, err := NewMetadata(&Version{}, tt.envs, newCommitResolverStub(), time.Now)
+			meta, err := NewMetadata(&Version{}, tt.envs, newCommitResolverStub(), time.Now, &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -315,7 +315,7 @@ func Test_buildkiteMetadata_initEnvData_extraFields(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			meta := buildkiteMetadata{}
-			err := meta.initEnvData(tt.envs, newCommitResolverStub())
+			err := meta.initEnvData(tt.envs, newCommitResolverStub(), &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -359,7 +359,7 @@ func Test_circleMetadata_initEnvData_extraFields(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			meta := circleMetadata{}
-			err := meta.initEnvData(tt.envs, newCommitResolverStub())
+			err := meta.initEnvData(tt.envs, newCommitResolverStub(), &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -400,7 +400,7 @@ func Test_githubMetadata_initEnvData_refTypes(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			meta := githubMetadata{}
-			err := meta.initEnvData(tt.envs, newCommitResolverStub())
+			err := meta.initEnvData(tt.envs, newCommitResolverStub(), &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -449,7 +449,7 @@ func Test_travisMetadata_initEnvData_extraFields(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			meta := travisMetadata{}
-			err := meta.initEnvData(tt.envs, newCommitResolverStub())
+			err := meta.initEnvData(tt.envs, newCommitResolverStub(), &stubLogger{})
 			assert.NoError(t, err)
 
 			yaml, err := meta.MarshalYAML()
@@ -492,3 +492,8 @@ func newCommitResolverStub() CommitResolver {
 			return &Commit{}, nil
 		})
 }
+
+// TODO: Look for better way to create a no-op logger for testing
+type stubLogger struct{}
+
+func (s *stubLogger) Printf(format string, v ...interface{}) {}

Original file line number	Diff line number	Diff line change
`@@ -143,7 +143,7 @@ func (s *Submit) Init(args []string, envs map[string]string) error {`
`143`	`143`	`// Run packages up the test results and sends them to BuildPulse. It returns the`
`144`	`144`	`// key that uniquely identifies the uploaded object.`
`145`	`145`	`func (s *Submit) Run() (string, error) {`
`146`		`- meta, err := metadata.NewMetadata(s.version, s.envs, s.commitResolver, time.Now)`
	`146`	`+ meta, err := metadata.NewMetadata(s.version, s.envs, s.commitResolver, time.Now, s.diagnostics)`
`147`	`147`	`if err != nil {`
`148`	`148`	`return "", err`
`149`	`149`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@ package metadata`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"fmt"`
`5`		`- "os"`
`6`	`5`	`"regexp"`
`7`	`6`	`"strconv"`
`8`	`7`	`"strings"`
`@@ -18,11 +17,16 @@ import (`
`18`	`17`	`type Metadata interface {`
`19`	`18`	`MarshalYAML() (out []byte, err error)`
`20`	`19`
`21`		`- initEnvData(envs map[string]string, resolver CommitResolver) error`
	`20`	`+ initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error`
`22`	`21`	`initTimestamp(now func() time.Time)`
`23`	`22`	`initVersionData(version *Version)`
`24`	`23`	`}`
`25`	`24`
	`25`	`+// Logger -- TODO Add docs`
	`26`	`+type Logger interface {`
	`27`	`+ Printf(format string, v ...interface{})`
	`28`	`+}`
	`29`	`+`
`26`	`30`	`// AbstractMetadata provides the fields that are common across all Metadata`
`27`	`31`	`// instances, regardless of the specific CI provider.`
`28`	`32`	`type AbstractMetadata struct {`
`@@ -45,10 +49,10 @@ type AbstractMetadata struct {`
`45`	`49`	TreeSHA string `yaml:":tree,omitempty"`
`46`	`50`	`}`
`47`	`51`
`48`		`-func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string) error {`
	`52`	`+func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string, log Logger) error {`
`49`	`53`	`// Git metadata functionality is experimental. While it's experimental, detect a nil CommitResolver and allow the commit metadata fields to be uploaded with empty values.`
`50`	`54`	`if cr == nil {`
`51`		`- fmt.Fprintf(os.Stderr, "[experimental] no commit resolver available; falling back to commit data from environment\n")`
	`55`	`+ log.Printf("[experimental] no commit resolver available; falling back to commit data from environment\n")`
`52`	`56`
`53`	`57`	`a.CommitSHA = sha`
`54`	`58`	`return nil`
`@@ -57,7 +61,7 @@ func (a *AbstractMetadata) initCommitData(cr CommitResolver, sha string) error {`
`57`	`61`	`// Git metadata functionality is experimental. While it's experimental, don't let this error prevent the test-reporter from continuing normal operation. Allow the commit metadata fields to be uploaded with empty values.`
`58`	`62`	`c, err := cr.Lookup(sha)`
`59`	`63`	`if err != nil {`
`60`		`- fmt.Fprintf(os.Stderr, "[experimental] git-based commit lookup unsuccessful; falling back to commit data from environment: %v\n", err)`
	`64`	`+ log.Printf("[experimental] git-based commit lookup unsuccessful; falling back to commit data from environment: %v\n", err)`
`61`	`65`
`62`	`66`	`a.CommitSHA = sha`
`63`	`67`	`return nil`
`@@ -86,7 +90,7 @@ func (a AbstractMetadata) initVersionData(version Version) {`
`86`	`90`	`}`
`87`	`91`
`88`	`92`	`// NewMetadata creates a new Metadata instance from the given args.`
`89`		`-func NewMetadata(version *Version, envs map[string]string, resolver CommitResolver, now func() time.Time) (Metadata, error) {`
	`93`	`+func NewMetadata(version *Version, envs map[string]string, resolver CommitResolver, now func() time.Time, log Logger) (Metadata, error) {`
`90`	`94`	`var m Metadata`
`91`	`95`
`92`	`96`	`switch {`
`@@ -106,7 +110,7 @@ func NewMetadata(version *Version, envs map[string]string, resolver CommitResolv`
`106`	`110`	`return nil, fmt.Errorf("unrecognized environment: system does not appear to be a supported CI provider (Buildkite, CircleCI, GitHub Actions, Jenkins, Semaphore, or Travis CI)")`
`107`	`111`	`}`
`108`	`112`
`109`		`- if err := m.initEnvData(envs, resolver); err != nil {`
	`113`	`+ if err := m.initEnvData(envs, resolver, log); err != nil {`
`110`	`114`	`return nil, err`
`111`	`115`	`}`
`112`	`116`	`m.initTimestamp(now)`
`@@ -142,12 +146,12 @@ type buildkiteMetadata struct {`
`142`	`146`	BuildkiteTag string `env:"BUILDKITE_TAG" yaml:":buildkite_tag,omitempty"`
`143`	`147`	`}`
`144`	`148`
`145`		`-func (b *buildkiteMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`149`	`+func (b *buildkiteMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`146`	`150`	`if err := env.Parse(b, env.Options{Environment: envs}); err != nil {`
`147`	`151`	`return err`
`148`	`152`	`}`
`149`	`153`
`150`		`- if err := b.initCommitData(resolver, b.BuildkiteCommit); err != nil {`
	`154`	`+ if err := b.initCommitData(resolver, b.BuildkiteCommit, log); err != nil {`
`151`	`155`	`return err`
`152`	`156`	`}`
`153`	`157`
`@@ -199,12 +203,12 @@ type circleMetadata struct {`
`199`	`203`	CircleWorkflowID string `env:"CIRCLE_WORKFLOW_ID" yaml:":circle_workflow_id"`
`200`	`204`	`}`
`201`	`205`
`202`		`-func (c *circleMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`206`	`+func (c *circleMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`203`	`207`	`if err := env.Parse(c, env.Options{Environment: envs}); err != nil {`
`204`	`208`	`return err`
`205`	`209`	`}`
`206`	`210`
`207`		`- if err := c.initCommitData(resolver, c.CircleSHA1); err != nil {`
	`211`	`+ if err := c.initCommitData(resolver, c.CircleSHA1, log); err != nil {`
`208`	`212`	`return err`
`209`	`213`	`}`
`210`	`214`
`@@ -243,12 +247,12 @@ type githubMetadata struct {`
`243`	`247`	GithubWorkflow string `env:"GITHUB_WORKFLOW" yaml:":github_workflow"`
`244`	`248`	`}`
`245`	`249`
`246`		`-func (g *githubMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`250`	`+func (g *githubMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`247`	`251`	`if err := env.Parse(g, env.Options{Environment: envs}); err != nil {`
`248`	`252`	`return err`
`249`	`253`	`}`
`250`	`254`
`251`		`- if err := g.initCommitData(resolver, g.GithubSHA); err != nil {`
	`255`	`+ if err := g.initCommitData(resolver, g.GithubSHA, log); err != nil {`
`252`	`256`	`return err`
`253`	`257`	`}`
`254`	`258`
`@@ -302,12 +306,12 @@ type jenkinsMetadata struct {`
`302`	`306`	JenkinsWorkspace string `env:"WORKSPACE" yaml:":jenkins_workspace"`
`303`	`307`	`}`
`304`	`308`
`305`		`-func (j *jenkinsMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`309`	`+func (j *jenkinsMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`306`	`310`	`if err := env.Parse(j, env.Options{Environment: envs}); err != nil {`
`307`	`311`	`return err`
`308`	`312`	`}`
`309`	`313`
`310`		`- if err := j.initCommitData(resolver, j.GitCommit); err != nil {`
	`314`	`+ if err := j.initCommitData(resolver, j.GitCommit, log); err != nil {`
`311`	`315`	`return err`
`312`	`316`	`}`
`313`	`317`
`@@ -363,12 +367,12 @@ type semaphoreMetadata struct {`
`363`	`367`	SemaphoreWorkflowNumber uint `env:"SEMAPHORE_WORKFLOW_NUMBER" yaml:":semaphore_workflow_number"`
`364`	`368`	`}`
`365`	`369`
`366`		`-func (s *semaphoreMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`370`	`+func (s *semaphoreMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`367`	`371`	`if err := env.Parse(s, env.Options{Environment: envs}); err != nil {`
`368`	`372`	`return err`
`369`	`373`	`}`
`370`	`374`
`371`		`- if err := s.initCommitData(resolver, s.SemaphoreGitSHA); err != nil {`
	`375`	`+ if err := s.initCommitData(resolver, s.SemaphoreGitSHA, log); err != nil {`
`372`	`376`	`return err`
`373`	`377`	`}`
`374`	`378`
`@@ -419,12 +423,12 @@ type travisMetadata struct {`
`419`	`423`	TravisTestResult uint `env:"TRAVIS_TEST_RESULT" yaml:":travis_test_result"`
`420`	`424`	`}`
`421`	`425`
`422`		`-func (t *travisMetadata) initEnvData(envs map[string]string, resolver CommitResolver) error {`
	`426`	`+func (t *travisMetadata) initEnvData(envs map[string]string, resolver CommitResolver, log Logger) error {`
`423`	`427`	`if err := env.Parse(t, env.Options{Environment: envs}); err != nil {`
`424`	`428`	`return err`
`425`	`429`	`}`
`426`	`430`
`427`		`- if err := t.initCommitData(resolver, t.TravisCommit); err != nil {`
	`431`	`+ if err := t.initCommitData(resolver, t.TravisCommit, log); err != nil {`
`428`	`432`	`return err`
`429`	`433`	`}`
`430`	`434`