Skip to content

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

Closed
wants to merge 4 commits into from

Conversation

dwahler
Copy link
Contributor

@dwahler dwahler commented Jul 14, 2022

This PR removes the existing functionality in the agent that was setting GIT_* environment variables, and instead automatically configures the user.name and user.email global config settings so that users can customize the values.

Making this testable required a bit more complexity than one might have expected. For one thing, we want to modify the actual ~/.gitconfig file when running a real agent but not when running tests, so I added a GitConfigPath field to the agent metadata object. For another, the test case covers modification of an existing Git config file, so agent.New now returns an interface that allows detecting when the agent has initialized itself, and agent_test.setupAgent waits for that condition.

Fixes #2665, obsoletes #2980

Comment on lines 219 to 236
t.Run("GitAutoconfig", func(t *testing.T) {
t.Parallel()
configPath := filepath.Join(os.TempDir(), "gitconfig")

initialContent := "[user]\nemail = elmo@example.com\n"
err := os.WriteFile(configPath, []byte(initialContent), 0600)
require.NoError(t, err)

setupAgent(t, agent.Metadata{
OwnerUsername: "Kermit the Frog",
OwnerEmail: "kermit@example.com",
GitConfigPath: configPath,
}, 0)

gotContent := readFileContents(t, configPath)
require.Contains(t, gotContent, "name = Kermit the Frog")
require.Contains(t, gotContent, "email = elmo@example.com")
})
Copy link
Member

Choose a reason for hiding this comment

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

praise: nice unit test!

if configPath == "" {
return nil
}
if strings.HasPrefix(configPath, "~/") {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ~/.gitconfig from the agent regardless of OS.

I guess it would be a bit cleaner (though more verbose) to turn configPath into an object that directly indicates whether it's absolute or relative to the home dir.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I just have a few questions about the path handling on Windows.

LGTM otherwise 👍


var errNoGitAvailable = xerrors.New("Git does not seem to be installed")

func setupGitconfig(ctx context.Context, configPath string, params map[string]string) error {
Copy link
Member

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.

Copy link
Contributor Author

@dwahler dwahler Jul 14, 2022

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?

git config --global user.name ${data.coder_workspace.me.owner}
git config --global user.email ${data.coder_workspace.me.owner_email}

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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:

  1. bash -l 'checkout dotfiles; ./dotfiles/setup'
  2. zsh -l 'git config --global user.name ...'
  3. ...

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 as bash, but git config is run as zsh.

Copy link
Member

@kylecarbs kylecarbs Jul 18, 2022

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:

resource "coder_agent" "testing" {
  env {
    GIT_AUTHOR_NAME = "${data.coder_workspace.me.owner_name}"
    GIT_AUTHOR_EMAIL = "${data.coder_workspace.me.owner_email}"
  }
}

This can just be a note in our docs for guidance.

@dwahler
Copy link
Contributor Author

dwahler commented Jul 19, 2022

Closing because we decided to go with the approach in #3034 instead.

@dwahler dwahler closed this Jul 19, 2022
@github-actions github-actions bot deleted the dwahler/2665-automatic-gitconfig branch February 4, 2023 20:04
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.

Add option to disable the automated settings of git env vars or make it possible to customise them
5 participants