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.

Sign up for GitHub

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
Closed

Automatically configure user.name and user.email in .gitconfig #2981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e75526
expose SetupComplete() method on agent.agent
dwahler Jul 14, 2022
5a2ea9b
provide default git configuration in .gitconfig instead of environmen…
dwahler Jul 14, 2022
1fe721a
don't try to interpret .gitconfig as UTF-16 on Window
dwahler Jul 14, 2022
a3a6ce5
use filepath.Join instead of string concatenation
dwahler Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ const (
ProtocolDial = "dial"
)

type Agent interface {
io.Closer
SetupComplete() bool
}

type Options struct {
EnableWireguard bool
UploadWireguardKeys UploadWireguardKeys
Expand All @@ -61,6 +66,7 @@ type Metadata struct {
EnvironmentVariables map[string]string `json:"environment_variables"`
StartupScript string `json:"startup_script"`
Directory string `json:"directory"`
GitConfigPath string `json:"git_config_path"`
}

type WireguardPublicKeys struct {
Expand All @@ -72,7 +78,7 @@ type Dialer func(ctx context.Context, logger slog.Logger) (Metadata, *peerbroker
type UploadWireguardKeys func(ctx context.Context, keys WireguardPublicKeys) error
type ListenWireguardPeers func(ctx context.Context, logger slog.Logger) (<-chan peerwg.Handshake, func(), error)

func New(dialer Dialer, options *Options) io.Closer {
func New(dialer Dialer, options *Options) Agent {
if options == nil {
options = &Options{}
}
Expand Down Expand Up @@ -109,8 +115,10 @@ type agent struct {

envVars map[string]string
// metadata is atomic because values can change after reconnection.
metadata atomic.Value
startupScript atomic.Bool
metadata atomic.Value
// tracks whether or not we have started/completed initial setup, including any startup script
setupStarted atomic.Bool
setupComplete atomic.Bool
sshServer *ssh.Server

enableWireguard bool
Expand Down Expand Up @@ -147,15 +155,16 @@ func (a *agent) run(ctx context.Context) {
}
a.metadata.Store(metadata)

if a.startupScript.CAS(false, true) {
if a.setupStarted.CAS(false, true) {
// The startup script has not ran yet!
go func() {
err := a.runStartupScript(ctx, metadata.StartupScript)
defer a.setupComplete.Store(true)
err := a.performInitialSetup(ctx, &metadata)
if errors.Is(err, context.Canceled) {
return
}
if err != nil {
a.logger.Warn(ctx, "agent script failed", slog.Error(err))
a.logger.Warn(ctx, "initial setup failed", slog.Error(err))
}
}()
}
Expand Down Expand Up @@ -184,6 +193,26 @@ func (a *agent) run(ctx context.Context) {
}
}

func (a *agent) performInitialSetup(ctx context.Context, metadata *Metadata) error {
err := setupGitconfig(ctx, metadata.GitConfigPath, map[string]string{
"user.name": metadata.OwnerUsername,
"user.email": metadata.OwnerEmail,
})
if errors.Is(err, context.Canceled) {
return err
}
if err != nil {
// failure to set up gitconfig shouldn't prevent the startup script from running
a.logger.Warn(ctx, "git autoconfiguration failed", slog.Error(err))
}

err = a.runStartupScript(ctx, metadata.StartupScript)
if err != nil {
return xerrors.Errorf("agent script failed: %w", err)
}
return nil
}

func (a *agent) runStartupScript(ctx context.Context, script string) error {
if script == "" {
return nil
Expand Down Expand Up @@ -386,12 +415,6 @@ func (a *agent) createCommand(ctx context.Context, rawCommand string, env []stri
// If using backslashes, it's unable to find the executable.
unixExecutablePath := strings.ReplaceAll(executablePath, "\\", "/")
cmd.Env = append(cmd.Env, fmt.Sprintf(`GIT_SSH_COMMAND=%s gitssh --`, unixExecutablePath))
// These prevent the user from having to specify _anything_ to successfully commit.
// Both author and committer must be set!
cmd.Env = append(cmd.Env, fmt.Sprintf(`GIT_AUTHOR_EMAIL=%s`, metadata.OwnerEmail))
cmd.Env = append(cmd.Env, fmt.Sprintf(`GIT_COMMITTER_EMAIL=%s`, metadata.OwnerEmail))
cmd.Env = append(cmd.Env, fmt.Sprintf(`GIT_AUTHOR_NAME=%s`, metadata.OwnerUsername))
cmd.Env = append(cmd.Env, fmt.Sprintf(`GIT_COMMITTER_NAME=%s`, metadata.OwnerUsername))

// Load environment variables passed via the agent.
// These should override all variables we manually specify.
Expand Down Expand Up @@ -755,6 +778,10 @@ func (a *agent) Close() error {
return nil
}

func (a *agent) SetupComplete() bool {
return a.setupComplete.Load()
}

type reconnectingPTY struct {
activeConnsMutex sync.Mutex
activeConns map[string]net.Conn
Expand Down
51 changes: 32 additions & 19 deletions agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,26 +212,38 @@ func TestAgent(t *testing.T) {
StartupScript: fmt.Sprintf("echo %s > %s", content, tempPath),
}, 0)

var gotContent string
require.Eventually(t, func() bool {
content, err := os.ReadFile(tempPath)
if err != nil {
return false
}
if len(content) == 0 {
return false
}
if runtime.GOOS == "windows" {
// Windows uses UTF16! 🪟🪟🪟
content, _, err = transform.Bytes(unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewDecoder(), content)
require.NoError(t, err)
}
gotContent = string(content)
return true
}, 15*time.Second, 100*time.Millisecond)
gotContentBytes, err := os.ReadFile(tempPath)
require.NoError(t, err)
if runtime.GOOS == "windows" {
// Windows uses UTF16! 🪟🪟🪟
gotContentBytes, _, err = transform.Bytes(unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewDecoder(), gotContentBytes)
require.NoError(t, err)
}
gotContent := string(gotContentBytes)
require.Equal(t, content, strings.TrimSpace(gotContent))
})

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)

gotContentBytes, err := os.ReadFile(configPath)
require.NoError(t, err)
gotContent := string(gotContentBytes)
require.Contains(t, gotContent, "name = Kermit the Frog")
require.Contains(t, gotContent, "email = elmo@example.com")
})

t.Run("ReconnectingPTY", func(t *testing.T) {
t.Parallel()
if runtime.GOOS == "windows" {
Expand Down Expand Up @@ -436,7 +448,7 @@ func setupSSHSession(t *testing.T, options agent.Metadata) *ssh.Session {

func setupAgent(t *testing.T, metadata agent.Metadata, ptyTimeout time.Duration) *agent.Conn {
client, server := provisionersdk.TransportPipe()
closer := agent.New(func(ctx context.Context, logger slog.Logger) (agent.Metadata, *peerbroker.Listener, error) {
a := agent.New(func(ctx context.Context, logger slog.Logger) (agent.Metadata, *peerbroker.Listener, error) {
listener, err := peerbroker.Listen(server, nil)
return metadata, listener, err
}, &agent.Options{
Expand All @@ -446,7 +458,7 @@ func setupAgent(t *testing.T, metadata agent.Metadata, ptyTimeout time.Duration)
t.Cleanup(func() {
_ = client.Close()
_ = server.Close()
_ = closer.Close()
_ = a.Close()
})
api := proto.NewDRPCPeerBrokerClient(provisionersdk.Conn(client))
stream, err := api.NegotiateConnection(context.Background())
Expand All @@ -458,6 +470,7 @@ func setupAgent(t *testing.T, metadata agent.Metadata, ptyTimeout time.Duration)
t.Cleanup(func() {
_ = conn.Close()
})
require.Eventually(t, a.SetupComplete, 10*time.Second, 100*time.Millisecond)

return &agent.Conn{
Negotiator: api,
Expand Down
60 changes: 60 additions & 0 deletions agent/gitconfig.go
View file
Open in desktop
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 {
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 •
edited
Loading

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 •
edited
Loading

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.

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.

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
}
1 change: 1 addition & 0 deletions coderd/workspaceagents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ func (api *API) workspaceAgentMetadata(rw http.ResponseWriter, r *http.Request)
EnvironmentVariables: apiAgent.EnvironmentVariables,
StartupScript: apiAgent.StartupScript,
Directory: apiAgent.Directory,
GitConfigPath: "~/.gitconfig",
})
}

Expand Down