Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #299 #300

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Fixes #299 #300

merged 1 commit into from
Mar 17, 2023

Conversation

nl-brett-stime
Copy link
Contributor

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

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

hashicorp-cla commented Jan 6, 2021

CLA assistant check
All committers have signed the CLA.

@okgolove
Copy link

Any chances this one will be merged?

@DanHam
Copy link

DanHam commented Mar 5, 2022

Please merge!

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

}

env = append(env, strings.Join(sshCmd, " "))
cmd.Env = env
}
Copy link
Contributor Author

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

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
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, " "))
}

Copy link

@theherk theherk left a comment

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

@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

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

Copy link
Member

@kmoe kmoe left a comment

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