Skip to content

Fixes #299 - #300

Merged
kmoe merged 1 commit into
hashicorp:mainfrom
nl-brett-stime:patch-1
Mar 17, 2023
Merged

kmoe merged 1 commit into
hashicorp:mainfrom
nl-brett-stime:patch-1

Conversation

@nl-brett-stime

Copy link
Copy Markdown
Contributor

Does not override GIT_SSH_COMMAND when not needed. See #299 .

Does not override GIT_SSH_COMMAND when not needed.
@hashicorp-cla

hashicorp-cla commented Jan 6, 2021 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@okgolove

Copy link
Copy Markdown

Any chances this one will be merged?

@DanHam

DanHam commented Mar 5, 2022

Copy link
Copy Markdown

Please merge!

This would allow hashicorp/terraform#28968 to be closed!

Comment thread get_git.go

env = append(env, strings.Join(sshCmd, " "))
cmd.Env = env
}

@nl-brett-stime nl-brett-stime Mar 7, 2022 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please expand/unfold the lines above so that you can see the whole setupGitEnv function--it's not long.

The only place sshCmd is modified, and would therefore need to be written back to the environment, is if sshKeyFile != "" (line 263). Apart from that case, there's currently no reason to initialize a default sshCmd as in these lines from above (259-261):

	if len(sshCmd) == 0 {
		sshCmd = []string{gitSSHCommand + "ssh"}
	}

If you feel like there might be other types of modifications/mutations in the future (besides sshKeyFile), and you want to make it relatively clear/easy how to add them, then I'd recommend adding a variable like hasSshCmdBeenModified bool. E.g.,

var hasSshCmdBeenModified = false
if len(sshCmd) == 0 {
	// Ensure a default command is initialized in case there are any subsequent modifications
	sshCmd = []string{gitSSHCommand + "ssh"}
}
if sshKeyFile != "" {
	sshCmd = addKeyModification(sshCmd, sshKeyFile)
	hasSshCmdBeenModified = true
}
if shouldDoModificationB {
	sshCmd = doModificationB(sshCmd)
	hasSshCmdBeenModified = true
}
if shouldDoModificationC {
	sshCmd = doModificationC(sshCmd)
	hasSshCmdBeenModified = true
}
if hasSshCmdBeenModified {
	// Don't append unless there was an actual modification.
	// The value of sshCmd might be the temporary default that was prepared
	// for modifications but didn't end up being needed.
	env = append(env, strings.Join(sshCmd, " "))
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

...or maybe:

baseSshCmdForModification = []string{gitSSHCommand + "ssh"}
if len(sshCmd) == 0 {
	// Ensure a default command is initialized in case there are any subsequent modifications
	sshCmd = baseSshCmdForModification
}
if sshKeyFile != "" {
	sshCmd = addKeyModification(sshCmd, sshKeyFile)
}
if shouldDoModificationB {
	sshCmd = doModificationB(sshCmd)
}
if shouldDoModificationC {
	sshCmd = doModificationC(sshCmd)
}
if sshCmd != baseSshCmdForModification {
	env = append(env, strings.Join(maybeModifiedSshCmd, " "))
}

@theherk theherk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This would save having to export GIT_SSH_COMMAND in the environment regularly and fixes hashicorp/terraform#28968. This has been open for a bit, and merging would benefit at least a few of us.

@maunzCache

Copy link
Copy Markdown

@nywilken @claire-labry @kmoe @dpowley Any chance to give this some love? I'd much appreciate the merge or at least a comment why this cannot be merged.

@maunzCache

Copy link
Copy Markdown

@claire-labry @picatz You were the latest who merged their branches to the current master. Any chance that you can endorse this one?

@kmoe kmoe left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your patience. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants