From b9af8c4555fe32181a58160b7bec58d68981e80b Mon Sep 17 00:00:00 2001 From: Devashish Date: Wed, 17 Jul 2024 17:15:14 +0530 Subject: [PATCH] Add PR review suggestions --- go.mod | 1 + internal/hcp/registry/hcl.go | 4 +- internal/hcp/registry/json.go | 4 +- .../{metadata.cicd.go => metadata/cicd.go} | 37 +++++------ .../{metadata.os.go => metadata/os.go} | 61 +++++++++--------- internal/hcp/registry/metadata/os_test.go | 64 +++++++++++++++++++ .../{metadata.vcs.go => metadata/vcs.go} | 24 +++---- internal/hcp/registry/types.metadata_store.go | 8 ++- 8 files changed, 137 insertions(+), 66 deletions(-) rename internal/hcp/registry/{metadata.cicd.go => metadata/cicd.go} (71%) rename internal/hcp/registry/{metadata.os.go => metadata/os.go} (59%) create mode 100644 internal/hcp/registry/metadata/os_test.go rename internal/hcp/registry/{metadata.vcs.go => metadata/vcs.go} (89%) diff --git a/go.mod b/go.mod index 43e634339..9c4afc1dc 100644 --- a/go.mod +++ b/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 diff --git a/internal/hcp/registry/hcl.go b/internal/hcp/registry/hcl.go index e4668ea1d..d512e3c0d 100644 --- a/internal/hcp/registry/hcl.go +++ b/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 } diff --git a/internal/hcp/registry/json.go b/internal/hcp/registry/json.go index 1ce5980d4..cddeae91f 100644 --- a/internal/hcp/registry/json.go +++ b/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 } diff --git a/internal/hcp/registry/metadata.cicd.go b/internal/hcp/registry/metadata/cicd.go similarity index 71% rename from internal/hcp/registry/metadata.cicd.go rename to internal/hcp/registry/metadata/cicd.go index af1abf249..85dd61e63 100644 --- a/internal/hcp/registry/metadata.cicd.go +++ b/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(), } } } diff --git a/internal/hcp/registry/metadata.os.go b/internal/hcp/registry/metadata/os.go similarity index 59% rename from internal/hcp/registry/metadata.os.go rename to internal/hcp/registry/metadata/os.go index eb262cec8..ed09768f7 100644 --- a/internal/hcp/registry/metadata.os.go +++ b/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, "]") diff --git a/internal/hcp/registry/metadata/os_test.go b/internal/hcp/registry/metadata/os_test.go new file mode 100644 index 000000000..789449e8d --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/hcp/registry/metadata.vcs.go b/internal/hcp/registry/metadata/vcs.go similarity index 89% rename from internal/hcp/registry/metadata.vcs.go rename to internal/hcp/registry/metadata/vcs.go index 53b90316b..3084755a5 100644 --- a/internal/hcp/registry/metadata.vcs.go +++ b/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, >.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(), diff --git a/internal/hcp/registry/types.metadata_store.go b/internal/hcp/registry/types.metadata_store.go index bb910ee0f..8ef5e5777 100644 --- a/internal/hcp/registry/types.metadata_store.go +++ b/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 }