-
Notifications
You must be signed in to change notification settings - Fork 937
Automatically configure user.name and user.email in .gitconfig #2981
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
Changes from all commits
1e75526
5a2ea9b
1fe721a
a3a6ce5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
package agent | ||
|
||
import ( | ||
"context" | ||
"os/exec" | ||
"os/user" | ||
"path/filepath" | ||
"strings" | ||
|
||
"golang.org/x/xerrors" | ||
) | ||
|
||
var errNoGitAvailable = xerrors.New("Git does not seem to be installed") | ||
|
||
func setupGitconfig(ctx context.Context, configPath string, params map[string]string) error { | ||
if configPath == "" { | ||
return nil | ||
} | ||
if strings.HasPrefix(configPath, "~/") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work on Windows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's admittedly a bit janky, but it does work, because we're passing the literal string I guess it would be a bit cleaner (though more verbose) to turn |
||
currentUser, err := user.Current() | ||
if err != nil { | ||
return xerrors.Errorf("get current user: %w", err) | ||
} | ||
configPath = filepath.Join(currentUser.HomeDir, configPath[2:]) | ||
} | ||
|
||
cmd := exec.CommandContext(ctx, "git", "--version") | ||
err := cmd.Run() | ||
if err != nil { | ||
return errNoGitAvailable | ||
} | ||
|
||
for name, value := range params { | ||
err = setGitConfigIfUnset(ctx, configPath, name, value) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func setGitConfigIfUnset(ctx context.Context, configPath, name, value string) error { | ||
cmd := exec.CommandContext(ctx, "git", "config", "--file", configPath, "--get", name) | ||
err := cmd.Run() | ||
if err == nil { | ||
// an exit status of 0 means the value exists, so there's nothing to do | ||
return nil | ||
} | ||
// an exit status of 1 means the value is unset | ||
if cmd.ProcessState.ExitCode() != 1 { | ||
return xerrors.Errorf("getting %s: %w", name, err) | ||
} | ||
|
||
cmd = exec.CommandContext(ctx, "git", "config", "--file", configPath, "--add", name, value) | ||
_, err = cmd.Output() | ||
if err != nil { | ||
return xerrors.Errorf("setting %s=%s: %w", name, value, err) | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced we should be touching anything in the user's home directory like this. I'd be in favor of removing these variables and exposing
owner_email
as a variable in the Terraform. These variables are easy to templatize anyways, and I think this could have unexpected consequences.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. So are you suggesting putting something like this into the Terraform startup script?
One of the goals discussed in #2665 was to make this continue to work "automatically", but if it's handled by a short, easy-to-paste snippet that we put in our example templates, that's probably just as good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @kylecarbs that we probably shouldn't touch this file. For instance, this would break the use-case where a user stores their git config in
$XDG_CONFIG_HOME/git/config
(usually~/.config/git/config
) since~/.gitconfig
takes precedence, the user might not realize why their config is being ignored. It would be possible to detect the presence of~/.config/git/config
, but$XDG_CONFIG_HOME/git/config
requires evaluation of the users dotfiles (e.g. launching a login shell and outputting the value).Regarding adding
git config --global user.name ${data.coder_workspace.me.owner}
to the TF startup script, that could also suffer the same problem. If done like this, ideally it would be done in a step where the users environment is respected.It's unfortunate the env variables can't be used as fallback, they have the benefit of being outside the users configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we had the startup scripts/tasks in the terraform file that run after the agent starts. The git setup could be a configurable task rather than special thing we always run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be nice if we could specify task order, and checking out dotfiles would be part of this task order. Every task would run in the users full shell environment (e.g. login shell), then exit.
For instance:
bash -l 'checkout dotfiles; ./dotfiles/setup'
zsh -l 'git config --global user.name ...'
The
bash -l
part would be inferred from e.g./etc/passwd
(or some more robust method), this is because the users dotfiles might change the default shell between executions. So dotfiles are checked out asbash
, but git config is run aszsh
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mafredri I'm proposing a template author adds their own environment variables:
This can just be a note in our docs for guidance.