From 39dfcde2bf3fa2a01f80e061ce8b1f98db0bf84e Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Tue, 8 Aug 2023 16:38:49 -0700 Subject: [PATCH] refactor: simplify git_metadata_sha propigation this refactor is in reponse to the Slack comment "this resilient enough to lots of things that might go wrong?" reffering to the previous GitMetadataSHA implementation I agree it was not simple. I hope this is better. --- internal/acceptance/bake/bake_test.go | 6 +-- internal/builder/interpolator.go | 17 ++------ internal/builder/kiln_metadata.go | 4 +- internal/builder/metadata_git_sha.go | 60 ++++++++++++++------------- internal/commands/bake.go | 7 +++- internal/commands/bake_test.go | 9 ++-- 6 files changed, 52 insertions(+), 51 deletions(-) diff --git a/internal/acceptance/bake/bake_test.go b/internal/acceptance/bake/bake_test.go index 75acd0736..b520ee306 100644 --- a/internal/acceptance/bake/bake_test.go +++ b/internal/acceptance/bake/bake_test.go @@ -12,14 +12,14 @@ import ( "testing" "time" - "github.com/pivotal-cf/kiln/internal/builder" - "github.com/onsi/gomega/gbytes" "github.com/onsi/gomega/gexec" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" . "github.com/pivotal-cf-experimental/gomegamatchers" + + "github.com/pivotal-cf/kiln/internal/builder" ) const ( @@ -42,7 +42,7 @@ var _ = BeforeSuite(func() { pathToMain, err = gexec.Build("github.com/pivotal-cf/kiln", "--ldflags", fmt.Sprintf("-X main.version=%s", buildVersion)) Expect(err).NotTo(HaveOccurred()) - tileSourceRevision, err = builder.GitMetadataSHA(".", true)() + tileSourceRevision, err = builder.GitMetadataSHA(".", true) Expect(err).NotTo(HaveOccurred()) }) diff --git a/internal/builder/interpolator.go b/internal/builder/interpolator.go index 074d2045e..cf01db9ce 100644 --- a/internal/builder/interpolator.go +++ b/internal/builder/interpolator.go @@ -48,7 +48,7 @@ type InterpolateInput struct { PropertyBlueprints map[string]any RuntimeConfigs map[string]any StubReleases bool - MetadataGitSHA func() (string, error) + MetadataGitSHA string } func NewInterpolator() Interpolator { @@ -56,15 +56,6 @@ func NewInterpolator() Interpolator { } func (i Interpolator) Interpolate(input InterpolateInput, name string, templateYAML []byte) ([]byte, error) { - var gitMetadataSHA string - if input.MetadataGitSHA != nil { - sha, err := input.MetadataGitSHA() - if err != nil { - return nil, err - } - gitMetadataSHA = sha - } - interpolatedYAML, err := i.interpolate(input, name, templateYAML) if err != nil { return nil, err @@ -77,7 +68,7 @@ func (i Interpolator) Interpolate(input InterpolateInput, name string, templateY return setKilnMetadata(prettyMetadata, KilnMetadata{ KilnVersion: input.KilnVersion, - MetadataGitSHA: gitMetadataSHA, + MetadataGitSHA: input.MetadataGitSHA, }) } @@ -184,9 +175,7 @@ func (i Interpolator) functions(input InterpolateInput) template.FuncMap { if !ok { switch key { case MetadataGitSHAVariable: - if input.MetadataGitSHA != nil { - return input.MetadataGitSHA() - } + return input.MetadataGitSHA, nil case BuildVersionVariable: return versionFunc() } diff --git a/internal/builder/kiln_metadata.go b/internal/builder/kiln_metadata.go index ce620fcd9..749106dda 100644 --- a/internal/builder/kiln_metadata.go +++ b/internal/builder/kiln_metadata.go @@ -9,8 +9,8 @@ import ( ) type KilnMetadata struct { - MetadataGitSHA string `yaml:"metadata_git_sha,omitempty"` - KilnVersion string `yaml:"kiln_version,omitempty"` + MetadataGitSHA string `yaml:"metadata_git_sha"` + KilnVersion string `yaml:"kiln_version"` } func setKilnMetadata(in []byte, kilnMetadata KilnMetadata) ([]byte, error) { diff --git a/internal/builder/metadata_git_sha.go b/internal/builder/metadata_git_sha.go index f924ec2d2..56c5b3ba5 100644 --- a/internal/builder/metadata_git_sha.go +++ b/internal/builder/metadata_git_sha.go @@ -10,34 +10,38 @@ import ( const dirtyStateSHAValue = "DEVELOPMENT" -func GitMetadataSHA(repositoryDirectory string, isDev bool) func() (string, error) { - var cache string - return func() (s string, err error) { - if cache != "" { - return cache, nil - } - if _, err := exec.LookPath("git"); err != nil { - return "", fmt.Errorf("could not calculate %q: %w", MetadataGitSHAVariable, err) - } - gitStatus := exec.Command("git", "status", "--porcelain") - gitStatus.Dir = repositoryDirectory - err = gitStatus.Run() - if err != nil { - if gitStatus.ProcessState.ExitCode() == 1 && isDev { - _, _ = fmt.Fprintf(os.Stderr, "WARNING: git working directory has un-commited changes: the variable %q has has development only value %q", MetadataGitSHAVariable, dirtyStateSHAValue) - return "DEVELOPMENT", nil - } - return "", fmt.Errorf("failed to run `%s %s`: %w", gitStatus.Path, strings.Join(gitStatus.Args, " "), err) - } - var out bytes.Buffer - gitRevParseHead := exec.Command("git", "rev-parse", "HEAD") - gitRevParseHead.Dir = repositoryDirectory - gitRevParseHead.Stdout = &out - err = gitRevParseHead.Run() - if err != nil { - return "", fmt.Errorf("failed to get HEAD revision hash: %w", err) +func GitMetadataSHA(repositoryDirectory string, isDev bool) (string, error) { + if err := ensureGitExecutableIsFound(); err != nil { + return "", err + } + gitStatus := exec.Command("git", "status", "--porcelain") + gitStatus.Dir = repositoryDirectory + err := gitStatus.Run() + if err != nil { + if gitStatus.ProcessState.ExitCode() == 1 && isDev { + _, _ = fmt.Fprintf(os.Stderr, "WARNING: git working directory has un-commited changes: the variable %q has has development only value %q", MetadataGitSHAVariable, dirtyStateSHAValue) + return dirtyStateSHAValue, nil } - cache = strings.TrimSpace(out.String()) - return cache, nil + return "", fmt.Errorf("failed to run `%s %s`: %w", gitStatus.Path, strings.Join(gitStatus.Args, " "), err) + } + return gitHeadRevision(repositoryDirectory) +} + +func gitHeadRevision(repositoryDirectory string) (string, error) { + var out bytes.Buffer + gitRevParseHead := exec.Command("git", "rev-parse", "HEAD") + gitRevParseHead.Dir = repositoryDirectory + gitRevParseHead.Stdout = &out + err := gitRevParseHead.Run() + if err != nil { + return "", fmt.Errorf("failed to get HEAD revision hash: %w", err) + } + return strings.TrimSpace(out.String()), nil +} + +func ensureGitExecutableIsFound() error { + if _, err := exec.LookPath("git"); err != nil { + return fmt.Errorf("could not calculate %q: %w", MetadataGitSHAVariable, err) } + return nil } diff --git a/internal/commands/bake.go b/internal/commands/bake.go index 75a6e15e2..88dd7ebdd 100644 --- a/internal/commands/bake.go +++ b/internal/commands/bake.go @@ -456,6 +456,11 @@ func (b Bake) Execute(args []string) error { return fmt.Errorf("failed to read metadata: %s", err) } + gitMetadataSHA, err := builder.GitMetadataSHA(filepath.Dir(b.Options.Kilnfile), b.Options.MetadataOnly || b.Options.StubReleases) + if err != nil { + return fmt.Errorf("failed to read metadata: %s", err) + } + input := builder.InterpolateInput{ KilnVersion: b.KilnVersion, Version: b.Options.Version, @@ -471,7 +476,7 @@ func (b Bake) Execute(args []string) error { PropertyBlueprints: propertyBlueprints, RuntimeConfigs: runtimeConfigs, StubReleases: b.Options.StubReleases, - MetadataGitSHA: builder.GitMetadataSHA(filepath.Dir(b.Options.Kilnfile), b.Options.MetadataOnly || b.Options.StubReleases), + MetadataGitSHA: gitMetadataSHA, } interpolatedMetadata, err := b.interpolator.Interpolate(input, b.Options.Metadata, metadata) if err != nil { diff --git a/internal/commands/bake_test.go b/internal/commands/bake_test.go index 03e433cf5..a3aeb9a54 100644 --- a/internal/commands/bake_test.go +++ b/internal/commands/bake_test.go @@ -258,11 +258,14 @@ var _ = Describe("Bake", func() { Expect(fakeInterpolator.InterpolateCallCount()).To(Equal(1)) + metadataGitSHA, err := builder.GitMetadataSHA(".", true) + Expect(err).NotTo(HaveOccurred()) + input, interpolateName, metadata := fakeInterpolator.InterpolateArgsForCall(0) - Expect(input.MetadataGitSHA).NotTo(BeNil()) - input.MetadataGitSHA = nil // func pointers are not comparable unless they are both nil + Expect(input.MetadataGitSHA).NotTo(BeEmpty()) Expect(input).To(Equal(builder.InterpolateInput{ - Version: "1.2.3", + MetadataGitSHA: metadataGitSHA, + Version: "1.2.3", BOSHVariables: map[string]any{ "some-secret": builder.Metadata{ "name": "some-secret",