Add PR review suggestions

go.mod

@@ -166,6 +166,7 @@ require (
 	github.com/skeema/knownhosts v1.2.1 // indirect
 	github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
+	github.com/stretchr/objx v0.5.0 // indirect
 	github.com/tklauser/go-sysconf v0.3.11 // indirect
 	github.com/tklauser/numcpus v0.6.0 // indirect
 	github.com/ugorji/go/codec v1.2.6 // indirect

internal/hcp/registry/hcl.go

@@ -88,8 +88,8 @@ func (h *HCLRegistry) CompleteBuild(
 		buildName = cb.Type
 	}
 
-	buildMetadata := cb.GetMetadata()
-	err := h.bucket.Version.AddMetadataToBuild(ctx, buildName, buildMetadata, h.metadata)
+	buildMetadata, envMetadata := cb.GetMetadata(), h.metadata
+	err := h.bucket.Version.AddMetadataToBuild(ctx, buildName, buildMetadata, envMetadata)
 	if err != nil {
 		return nil, err
 	}

internal/hcp/registry/json.go

@@ -97,8 +97,8 @@ func (h *JSONRegistry) CompleteBuild(
 	buildErr error,
 ) ([]sdkpacker.Artifact, error) {
 	buildName := build.Name()
-	buildMetadata := build.(*packer.CoreBuild).GetMetadata()
-	err := h.bucket.Version.AddMetadataToBuild(ctx, buildName, buildMetadata, h.metadata)
+	buildMetadata, envMetadata := build.(*packer.CoreBuild).GetMetadata(),  h.metadata
+	err := h.bucket.Version.AddMetadataToBuild(ctx, buildName, buildMetadata, envMetadata)
 	if err != nil {
 		return nil, err
 	}

internal/hcp/registry/metadata/cicd.go

@@ -1,25 +1,22 @@
-package registry
+package metadata
 
 import (
 	"fmt"
 	"os"
 )
 
-type CICD interface {
-	Detect() bool
-	Env() map[string]string
-	Type() string
-}
-
 type GithubActions struct{}
 
-func (g *GithubActions) Detect() bool {
+func (g *GithubActions) Detect() error {
 	_, ok := os.LookupEnv("GITHUB_ACTIONS")
-	return ok
+	if !ok {
+		return fmt.Errorf("GITHUB_ACTIONS environment variable not found")
+	}
+	return nil
 }
 
-func (g *GithubActions) Env() map[string]string {
-	env := make(map[string]string)
+func (g *GithubActions) Details() map[string]interface{} {
+	env := make(map[string]interface{})
 	keys := []string{
 		"GITHUB_REPOSITORY",
 		"GITHUB_REPOSITORY_ID",
@@ -49,13 +46,16 @@ func (g *GithubActions) Type() string {
 
 type GitlabCI struct{}
 
-func (g *GitlabCI) Detect() bool {
+func (g *GitlabCI) Detect() error {
 	_, ok := os.LookupEnv("GITLAB_CI")
-	return ok
+	if !ok {
+		return fmt.Errorf("GITLAB_CI environment variable not found")
+	}
+	return nil
 }
 
-func (g *GitlabCI) Env() map[string]string {
-	env := make(map[string]string)
+func (g *GitlabCI) Details() map[string]interface{} {
+	env := make(map[string]interface{})
 	keys := []string{
 		"CI_PROJECT_NAME",
 		"CI_PROJECT_ID",
@@ -85,16 +85,17 @@ func (g *GitlabCI) Type() string {
 }
 
 func GetCicdMetadata() map[string]interface{} {
-	cicd := []CICD{
+	cicd := []MetadataProvider{
 		&GithubActions{},
 		&GitlabCI{},
 	}
 
 	for _, c := range cicd {
-		if c.Detect() {
+		err := c.Detect()
+		if err == nil {
 			return map[string]interface{}{
 				"type":    c.Type(),
-				"details": c.Env(),
+				"details": c.Details(),
 			}
 		}
 	}

internal/hcp/registry/metadata/os.go

@@ -1,7 +1,6 @@
-package registry
+package metadata
 
 import (
-	"bytes"
 	"log"
 	"os/exec"
 	"runtime"
@@ -15,22 +14,38 @@ type OSInfo struct {
 	Version string
 }
 
+// CommandExecutor is an interface for executing commands.
+type CommandExecutor interface {
+	Exec(name string, arg ...string) ([]byte, error)
+}
+
+// DefaultExecutor is the default implementation of CommandExecutor.
+type DefaultExecutor struct{}
+
+// Exec executes a command and returns the combined output.
+func (d DefaultExecutor) Exec(name string, arg ...string) ([]byte, error) {
+	cmd := exec.Command(name, arg...)
+	return cmd.CombinedOutput()
+}
+
+var executor CommandExecutor = DefaultExecutor{}
+
 func GetOSMetadata() map[string]interface{} {
 	var osInfo OSInfo
 
 	switch runtime.GOOS {
 	case "windows":
-		osInfo = GetInfoForWindows()
+		osInfo = GetInfoForWindows(executor)
 	case "darwin":
-		osInfo = GetInfo("-srm")
+		osInfo = GetInfo(executor, "-srm")
 	case "linux":
-		osInfo = GetInfo("-srio")
+		osInfo = GetInfo(executor, "-srio")
 	case "freebsd":
-		osInfo = GetInfo("-sri")
+		osInfo = GetInfo(executor, "-sri")
 	case "openbsd":
-		osInfo = GetInfo("-srm")
+		osInfo = GetInfo(executor, "-srm")
 	case "netbsd":
-		osInfo = GetInfo("-srm")
+		osInfo = GetInfo(executor, "-srm")
 	default:
 		osInfo = OSInfo{
 			Name: runtime.GOOS,
@@ -47,11 +62,11 @@ func GetOSMetadata() map[string]interface{} {
 	}
 }
 
-func GetInfo(flags string) OSInfo {
-	out, err := _uname(flags)
+func GetInfo(exec CommandExecutor, flags string) OSInfo {
+	out, err := _uname(exec, flags)
 	tries := 0
 	for strings.Contains(out, "broken pipe") && tries < 3 {
-		out, err = _uname(flags)
+		out, err = _uname(exec, flags)
 		time.Sleep(500 * time.Millisecond)
 		tries++
 	}
@@ -70,15 +85,9 @@ func GetInfo(flags string) OSInfo {
 	}
 }
 
-func _uname(flags string) (string, error) {
-	cmd := exec.Command("uname", flags)
-	cmd.Stdin = strings.NewReader("some input")
-	var out bytes.Buffer
-	var stderr bytes.Buffer
-	cmd.Stdout = &out
-	cmd.Stderr = &stderr
-	err := cmd.Run()
-	return out.String(), err
+func _uname(exec CommandExecutor, flags string) (string, error) {
+	output, err := exec.Exec("uname", flags)
+	return string(output), err
 }
 
 func _retrieveCore(osStr string) string {
@@ -93,14 +102,8 @@ func _retrieveCore(osStr string) string {
 	return core
 }
 
-func GetInfoForWindows() OSInfo {
-	cmd := exec.Command("cmd", "ver")
-	cmd.Stdin = strings.NewReader("some input")
-	var out bytes.Buffer
-	var stderr bytes.Buffer
-	cmd.Stdout = &out
-	cmd.Stderr = &stderr
-	err := cmd.Run()
+func GetInfoForWindows(exec CommandExecutor) OSInfo {
+	out, err := exec.Exec("cmd", "ver")
 	if err != nil {
 		log.Printf("[ERROR] failed to get the OS info: %s", err)
 		return OSInfo{
@@ -109,7 +112,7 @@ func GetInfoForWindows() OSInfo {
 		}
 	}
 
-	osStr := strings.Replace(out.String(), "\n", "", -1)
+	osStr := strings.Replace(string(out), "\n", "", -1)
 	osStr = strings.Replace(osStr, "\r\n", "", -1)
 	tmp1 := strings.Index(osStr, "[Version")
 	tmp2 := strings.Index(osStr, "]")

internal/hcp/registry/metadata/os_test.go

@@ -0,0 +1,64 @@
+package metadata
+
+import (
+	"fmt"
+	"runtime"
+	"testing"
+)
+
+// MockExecutor is a mock implementation of CommandExecutor.
+type MockExecutor struct {
+	stdout string
+	err    error
+}
+
+// Exec returns a mocked output.
+func (m MockExecutor) Exec(name string, arg ...string) ([]byte, error) {
+	return []byte(m.stdout), m.err
+}
+
+func TestGetInfoForWindows(t *testing.T) {
+	tests := []struct {
+		name     string
+		stdout   string
+		err   error
+		expected OSInfo
+	}{
+		{
+			name:     "Valid version info",
+			stdout:   "Microsoft Windows [Version 10.0.19042.928]",
+			err: nil,
+			expected: OSInfo{
+				Name:    runtime.GOOS,
+				Arch:    runtime.GOARCH,
+				Version: "10.0.19042.928",
+			},
+		},
+		{
+			name:     "Invalid version info",
+			stdout:   "Invalid output",
+			err:   fmt.Errorf("Invalid output"),
+			expected: OSInfo{
+				Name:    runtime.GOOS,
+				Arch:    runtime.GOARCH,
+				Version: "",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			mockExecutor := MockExecutor{
+				stdout: tt.stdout,
+				err: tt.err,
+			}
+
+			result := GetInfoForWindows(mockExecutor)
+
+			if result != tt.expected {
+				t.Errorf("expected %+v, got %+v", tt.expected, result)
+			}
+		})
+	}
+}

internal/hcp/registry/metadata/vcs.go

@@ -1,4 +1,4 @@
-package registry
+package metadata
 
 import (
 	"log"
@@ -7,8 +7,8 @@ import (
 	gt "github.com/go-git/go-git/v5"
 )
 
-type VCS interface {
-	Detect(wd string) error
+type MetadataProvider interface {
+	Detect() error
 	Details() map[string]interface{}
 	Type() string
 }
@@ -17,7 +17,13 @@ type Git struct {
 	repo *gt.Repository
 }
 
-func (g *Git) Detect(wd string) error {
+func (g *Git) Detect() error {
+	wd, err := os.Getwd()
+	if err != nil {
+		log.Printf("[ERROR] unable to retrieve current directory: %s", err)
+		return err
+	}
+
 	repo, err := gt.PlainOpenWithOptions(wd, &gt.PlainOpenOptions{DetectDotGit: true})
 	if err != nil {
 		return err
@@ -69,18 +75,12 @@ func (g *Git) Details() map[string]interface{} {
 }
 
 func GetVcsMetadata() map[string]interface{} {
-	wd, err := os.Getwd()
-	if err != nil {
-		log.Printf("[ERROR] unable to retrieve current directory: %s", err)
-		return map[string]interface{}{}
-	}
-
-	vcsSystems := []VCS{
+	vcsSystems := []MetadataProvider{
 		&Git{},
 	}
 
 	for _, vcs := range vcsSystems {
-		err := vcs.Detect(wd)
+		err := vcs.Detect()
 		if err == nil {
 			return map[string]interface{}{
 				"type":    vcs.Type(),

internal/hcp/registry/types.metadata_store.go

@@ -1,5 +1,7 @@
 package registry
 
+import "github.com/hashicorp/packer/internal/hcp/registry/metadata"
+
 // Metadata is the global metadata store, it is attached to a registry implementation
 // and keeps track of the environmental information.
 // This then can be sent to HCP Packer, so we can present it to users.
@@ -22,9 +24,9 @@ type MetadataStore struct {
 }
 
 func (ms *MetadataStore) Gather(args map[string]interface{}) {
-	ms.OperatingSystem = GetOSMetadata()
-	ms.Cicd = GetCicdMetadata()
-	ms.Vcs = GetVcsMetadata()
+	ms.OperatingSystem = metadata.GetOSMetadata()
+	ms.Cicd = metadata.GetCicdMetadata()
+	ms.Vcs = metadata.GetVcsMetadata()
 	ms.PackerBuildCommandOptions = args
 }