Skip to content

Remove last git shell-out and fix dropped commit label#4826

Merged
thardeck merged 4 commits into
mainfrom
remove_git_shell_out
Mar 17, 2026
Merged

Remove last git shell-out and fix dropped commit label#4826
thardeck merged 4 commits into
mainfrom
remove_git_shell_out

Conversation

@thardeck
Copy link
Copy Markdown
Collaborator

@thardeck thardeck commented Mar 16, 2026

This PR consolidates all git operations to go-git by replacing the last remaining shell-out to the git binary in production code. It also fixes a bug where the commit label is silently dropped when no other labels are set.

  • Replace exec.Command("git", "rev-parse", "HEAD") with go-git's PlainOpenWithOptions + ResolveRevision. Add dir parameter to currentCommit to enable unit testing from arbitrary directories, including nested subdirectories within a repo. DetectDotGit: true walks parent directories to locate the .git root, matching the shell-out behavior.
  • Fix bug in Apply.run where opts.Labels was set to a.Label instead of the local labels variable. When a.Label is nil but a commit is detected, labels is reassigned to a new map for the commit label — but opts.Labels remained nil, silently dropping the label. Now correctly uses labels.

Refers #4877

Give currentCommit a dir parameter so the logic can be tested with
arbitrary directories.
currentCommit called exec.Command("git", "rev-parse", "HEAD") to
read the HEAD SHA of the working directory. Replace with go-git's
PlainOpenWithOptions (with DetectDotGit to walk up to the .git root)
and ResolveRevision, removing the only remaining os/exec call in
production code.
@thardeck thardeck self-assigned this Mar 16, 2026
Copilot AI review requested due to automatic review settings March 16, 2026 13:52
@thardeck thardeck requested a review from a team as a code owner March 16, 2026 13:52
@thardeck thardeck added this to Fleet Mar 16, 2026
@thardeck thardeck added this to the v2.14.1 milestone Mar 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the last production dependency on the external git binary by switching fleet apply’s currentCommit implementation from a git rev-parse HEAD shell-out to using go-git APIs.

Changes:

  • Replace exec.Command("git", "rev-parse", "HEAD") with gogit.PlainOpenWithOptions + ResolveRevision("HEAD").
  • Update Apply.run to call currentCommit(".") with an explicit directory argument.
  • Add unit tests for currentCommit using an in-memory initialized go-git repo on disk.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
internal/cmd/cli/apply.go Replaces git shell-out with go-git open + revision resolve logic.
internal/cmd/cli/apply_test.go Adds TestCurrentCommit validating behavior for non-repo dirs and a simple repo HEAD.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread internal/cmd/cli/apply.go
Comment thread internal/cmd/cli/apply_test.go
DetectDotGit walks parent directories to locate the .git root,
matching the shell-out behavior. Add a subtest that calls
currentCommit from a nested path inside the repo to confirm this.
@thardeck thardeck requested a review from Copilot March 16, 2026 14:11
@thardeck thardeck changed the title Replace git shell-out with go-git in resolveHeadCommit Replace git shell-out with go-git in currentCommit() Mar 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the last production dependency on the git binary by switching fleet apply’s HEAD SHA detection from a git rev-parse HEAD exec to go-git’s repository APIs.

Changes:

  • Replaced shell-out (os/exec) logic in currentCommit with go-git PlainOpenWithOptions(...DetectDotGit) + ResolveRevision("HEAD").
  • Updated fleet apply to call currentCommit(".").
  • Added unit tests validating behavior in non-git dirs, git repos, and nested subdirectories.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/cmd/cli/apply.go Replaces exec-based HEAD resolution with go-git and updates call site to pass a directory.
internal/cmd/cli/apply_test.go Adds tests for currentCommit covering non-repo, repo, and nested directory cases.
Comments suppressed due to low confidence (1)

internal/cmd/cli/apply.go:109

  • labels is a local copy of a.Label. When a.Label is nil and a.Commit is set, this code initializes and mutates labels but never assigns it back to a.Label (or otherwise ensures it’s used later), so the commit label can be silently dropped. Consider assigning a.Label = labels after adding fleet.CommitLabel, or ensure the downstream options use labels instead of a.Label.
	labels := a.Label
	if a.Commit == "" {
		a.Commit = currentCommit(".")
	}
	if a.Commit != "" {
		if labels == nil {
			labels = map[string]string{}
		}
		labels[fleet.CommitLabel] = a.Commit

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

When a.Label is nil and a commit is detected, labels is reassigned
to a new map but opts.Labels was set to a.Label (still nil). Use
labels instead so the commit label is included in both the nil and
non-nil a.Label cases.
@thardeck thardeck changed the title Replace git shell-out with go-git in currentCommit() Remove last git shell-out and fix dropped commit label Mar 16, 2026
@thardeck thardeck moved this to 👀 In review in Fleet Mar 16, 2026
@thardeck thardeck merged commit 89428ea into main Mar 17, 2026
34 of 36 checks passed
@thardeck thardeck deleted the remove_git_shell_out branch March 17, 2026 09:41
@github-project-automation github-project-automation Bot moved this from 👀 In review to ✅ Done in Fleet Mar 17, 2026
@thardeck thardeck modified the milestones: v2.14.1, v2.15.0 Mar 17, 2026
@thardeck thardeck moved this from ✅ Done to Needs QA review in Fleet Mar 17, 2026
@kkaempf kkaempf moved this from Needs QA review to ✅ Done in Fleet Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants