Skip to content

Commit c3bc1e7

Browse files
DanielleMaywoodCopilot
DanielleMaywood
and
Copilot
authored
feat(agent/agentcontainers): fall back to workspace folder name (#18466)
This PR changes the logic for how we decide on an agent name. Previously it followed these steps: 1. Use a name from `customizations.coder.name` 2. Use a name from the terraform resource `coder_devcontainer` 3. Use the dev container's friendly name With this change it now does: 1. Use a name from `customizations.coder.name` 2. Use a name from the terraform resource `coder_devcontainer` 3. Use a name from the workspace folder 4. Use the dev container's friendly name We now attempt to construct a valid agent name from the workspace folder. Should we fail to construct a valid agent name from the workspace folder, we will fall back to the dev container's friendly name. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 0a12ec5 commit c3bc1e7

File tree

3 files changed

+255
-18
lines changed

3 files changed

+255
-18
lines changed

‎agent/agentcontainers/api.go

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"os"
99
"path"
1010
"path/filepath"
11+
"regexp"
1112
"runtime"
1213
"slices"
1314
"strings"
@@ -39,6 +40,8 @@ const (
3940
// by tmpfs or other mounts. This assumes the container root filesystem is
4041
// read-write, which seems sensible for devcontainers.
4142
coderPathInsideContainer = "/.coder-agent/coder"
43+
44+
maxAgentNameLength = 64
4245
)
4346

4447
// API is responsible for container-related operations in the agent.
@@ -583,10 +586,11 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code
583586
if dc.Container != nil {
584587
if !api.devcontainerNames[dc.Name] {
585588
// If the devcontainer name wasn't set via terraform, we
586-
// use the containers friendly name as a fallback which
587-
// will keep changing as the devcontainer is recreated.
588-
// TODO(mafredri): Parse the container label (i.e. devcontainer.json) for customization.
589-
dc.Name = safeFriendlyName(dc.Container.FriendlyName)
589+
// will attempt to create an agent name based on the workspace
590+
// folder's name. If it is not possible to generate a valid
591+
// agent name based off of the folder name (i.e. no valid characters),
592+
// we will instead fall back to using the container's friendly name.
593+
dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName)
590594
}
591595
}
592596

@@ -631,6 +635,38 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code
631635
api.containersErr = nil
632636
}
633637

638+
var consecutiveHyphenRegex = regexp.MustCompile("-+")
639+
640+
// `safeAgentName` returns a safe agent name derived from a folder name,
641+
// falling back to the container’s friendly name if needed.
642+
func safeAgentName(name string, friendlyName string) string {
643+
// Keep only ASCII letters and digits, replacing everything
644+
// else with a hyphen.
645+
var sb strings.Builder
646+
for _, r := range strings.ToLower(name) {
647+
if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') {
648+
_, _ = sb.WriteRune(r)
649+
} else {
650+
_, _ = sb.WriteRune('-')
651+
}
652+
}
653+
654+
// Remove any consecutive hyphens, and then trim any leading
655+
// and trailing hyphens.
656+
name = consecutiveHyphenRegex.ReplaceAllString(sb.String(), "-")
657+
name = strings.Trim(name, "-")
658+
659+
// Ensure the name of the agent doesn't exceed the maximum agent
660+
// name length.
661+
name = name[:min(len(name), maxAgentNameLength)]
662+
663+
if provisioner.AgentNameRegex.Match([]byte(name)) {
664+
return name
665+
}
666+
667+
return safeFriendlyName(friendlyName)
668+
}
669+
634670
// safeFriendlyName returns a API safe version of the container's
635671
// friendly name.
636672
//

‎agent/agentcontainers/api_internal_test.go

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
package agentcontainers
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
8+
"github.com/coder/coder/v2/provisioner"
9+
)
10+
11+
func TestSafeAgentName(t *testing.T) {
12+
t.Parallel()
13+
14+
tests := []struct {
15+
name string
16+
folderName string
17+
expected string
18+
}{
19+
// Basic valid names
20+
{
21+
folderName: "simple",
22+
expected: "simple",
23+
},
24+
{
25+
folderName: "with-hyphens",
26+
expected: "with-hyphens",
27+
},
28+
{
29+
folderName: "123numbers",
30+
expected: "123numbers",
31+
},
32+
{
33+
folderName: "mixed123",
34+
expected: "mixed123",
35+
},
36+
37+
// Names that need transformation
38+
{
39+
folderName: "With_Underscores",
40+
expected: "with-underscores",
41+
},
42+
{
43+
folderName: "With Spaces",
44+
expected: "with-spaces",
45+
},
46+
{
47+
folderName: "UPPERCASE",
48+
expected: "uppercase",
49+
},
50+
{
51+
folderName: "Mixed_Case-Name",
52+
expected: "mixed-case-name",
53+
},
54+
55+
// Names with special characters that get replaced
56+
{
57+
folderName: "special@#$chars",
58+
expected: "special-chars",
59+
},
60+
{
61+
folderName: "dots.and.more",
62+
expected: "dots-and-more",
63+
},
64+
{
65+
folderName: "multiple___underscores",
66+
expected: "multiple-underscores",
67+
},
68+
{
69+
folderName: "multiple---hyphens",
70+
expected: "multiple-hyphens",
71+
},
72+
73+
// Edge cases with leading/trailing special chars
74+
{
75+
folderName: "-leading-hyphen",
76+
expected: "leading-hyphen",
77+
},
78+
{
79+
folderName: "trailing-hyphen-",
80+
expected: "trailing-hyphen",
81+
},
82+
{
83+
folderName: "_leading_underscore",
84+
expected: "leading-underscore",
85+
},
86+
{
87+
folderName: "trailing_underscore_",
88+
expected: "trailing-underscore",
89+
},
90+
{
91+
folderName: "---multiple-leading",
92+
expected: "multiple-leading",
93+
},
94+
{
95+
folderName: "trailing-multiple---",
96+
expected: "trailing-multiple",
97+
},
98+
99+
// Complex transformation cases
100+
{
101+
folderName: "___very---complex@@@name___",
102+
expected: "very-complex-name",
103+
},
104+
{
105+
folderName: "my.project-folder_v2",
106+
expected: "my-project-folder-v2",
107+
},
108+
109+
// Empty and fallback cases - now correctly uses friendlyName fallback
110+
{
111+
folderName: "",
112+
expected: "friendly-fallback",
113+
},
114+
{
115+
folderName: "---",
116+
expected: "friendly-fallback",
117+
},
118+
{
119+
folderName: "___",
120+
expected: "friendly-fallback",
121+
},
122+
{
123+
folderName: "@#$",
124+
expected: "friendly-fallback",
125+
},
126+
127+
// Additional edge cases
128+
{
129+
folderName: "a",
130+
expected: "a",
131+
},
132+
{
133+
folderName: "1",
134+
expected: "1",
135+
},
136+
{
137+
folderName: "a1b2c3",
138+
expected: "a1b2c3",
139+
},
140+
{
141+
folderName: "CamelCase",
142+
expected: "camelcase",
143+
},
144+
{
145+
folderName: "snake_case_name",
146+
expected: "snake-case-name",
147+
},
148+
{
149+
folderName: "kebab-case-name",
150+
expected: "kebab-case-name",
151+
},
152+
{
153+
folderName: "mix3d_C4s3-N4m3",
154+
expected: "mix3d-c4s3-n4m3",
155+
},
156+
{
157+
folderName: "123-456-789",
158+
expected: "123-456-789",
159+
},
160+
{
161+
folderName: "abc123def456",
162+
expected: "abc123def456",
163+
},
164+
{
165+
folderName: " spaces everywhere ",
166+
expected: "spaces-everywhere",
167+
},
168+
{
169+
folderName: "unicode-café-naïve",
170+
expected: "unicode-caf-na-ve",
171+
},
172+
{
173+
folderName: "path/with/slashes",
174+
expected: "path-with-slashes",
175+
},
176+
{
177+
folderName: "file.tar.gz",
178+
expected: "file-tar-gz",
179+
},
180+
{
181+
folderName: "version-1.2.3-alpha",
182+
expected: "version-1-2-3-alpha",
183+
},
184+
185+
// Truncation test for names exceeding 64 characters
186+
{
187+
folderName: "this-is-a-very-long-folder-name-that-exceeds-sixty-four-characters-limit-and-should-be-truncated",
188+
expected: "this-is-a-very-long-folder-name-that-exceeds-sixty-four-characte",
189+
},
190+
}
191+
192+
for _, tt := range tests {
193+
t.Run(tt.folderName, func(t *testing.T) {
194+
t.Parallel()
195+
name := safeAgentName(tt.folderName, "friendly-fallback")
196+
197+
assert.Equal(t, tt.expected, name)
198+
assert.True(t, provisioner.AgentNameRegex.Match([]byte(name)))
199+
})
200+
}
201+
}

‎agent/agentcontainers/api_test.go

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -897,26 +897,26 @@ func TestAPI(t *testing.T) {
897897
FriendlyName: "project1-container",
898898
Running: true,
899899
Labels: map[string]string{
900-
agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project",
901-
agentcontainers.DevcontainerConfigFileLabel: "/workspace/project/.devcontainer/devcontainer.json",
900+
agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project1",
901+
agentcontainers.DevcontainerConfigFileLabel: "/workspace/project1/.devcontainer/devcontainer.json",
902902
},
903903
},
904904
{
905905
ID: "project2-container",
906906
FriendlyName: "project2-container",
907907
Running: true,
908908
Labels: map[string]string{
909-
agentcontainers.DevcontainerLocalFolderLabel: "/home/user/project",
910-
agentcontainers.DevcontainerConfigFileLabel: "/home/user/project/.devcontainer/devcontainer.json",
909+
agentcontainers.DevcontainerLocalFolderLabel: "/home/user/project2",
910+
agentcontainers.DevcontainerConfigFileLabel: "/home/user/project2/.devcontainer/devcontainer.json",
911911
},
912912
},
913913
{
914914
ID: "project3-container",
915915
FriendlyName: "project3-container",
916916
Running: true,
917917
Labels: map[string]string{
918-
agentcontainers.DevcontainerLocalFolderLabel: "/var/lib/project",
919-
agentcontainers.DevcontainerConfigFileLabel: "/var/lib/project/.devcontainer/devcontainer.json",
918+
agentcontainers.DevcontainerLocalFolderLabel: "/var/lib/project3",
919+
agentcontainers.DevcontainerConfigFileLabel: "/var/lib/project3/.devcontainer/devcontainer.json",
920920
},
921921
},
922922
},
@@ -1326,7 +1326,7 @@ func TestAPI(t *testing.T) {
13261326
// Allow initial agent creation and injection to succeed.
13271327
testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil)
13281328
testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error {
1329-
assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container")
1329+
assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=coder")
13301330
assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace")
13311331
assert.Contains(t, envs, "CODER_WORKSPACE_OWNER_NAME=test-user")
13321332
assert.Contains(t, envs, "CODER_URL=test-subagent-url")
@@ -1349,7 +1349,7 @@ func TestAPI(t *testing.T) {
13491349

13501350
// Verify agent was created.
13511351
require.Len(t, fakeSAC.created, 1)
1352-
assert.Equal(t, "test-container", fakeSAC.created[0].Name)
1352+
assert.Equal(t, "coder", fakeSAC.created[0].Name)
13531353
assert.Equal(t, "/workspaces/coder", fakeSAC.created[0].Directory)
13541354
assert.Len(t, fakeSAC.deleted, 0)
13551355

@@ -1405,7 +1405,7 @@ func TestAPI(t *testing.T) {
14051405
WaitStartLoop:
14061406
for {
14071407
// Agent reinjection will succeed and we will not re-create the
1408-
// agent, nor re-probe pwd.
1408+
// agent.
14091409
mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{
14101410
Containers: []codersdk.WorkspaceAgentContainer{testContainer},
14111411
}, nil).Times(1) // 1 update.
@@ -1468,7 +1468,7 @@ func TestAPI(t *testing.T) {
14681468
// Expect the agent to be recreated.
14691469
testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil)
14701470
testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error {
1471-
assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container")
1471+
assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=coder")
14721472
assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace")
14731473
assert.Contains(t, envs, "CODER_WORKSPACE_OWNER_NAME=test-user")
14741474
assert.Contains(t, envs, "CODER_URL=test-subagent-url")
@@ -1910,8 +1910,8 @@ func TestAPI(t *testing.T) {
19101910
Running: true,
19111911
CreatedAt: time.Now(),
19121912
Labels: map[string]string{
1913-
agentcontainers.DevcontainerLocalFolderLabel: "/workspaces",
1914-
agentcontainers.DevcontainerConfigFileLabel: "/workspace/.devcontainer/devcontainer.json",
1913+
agentcontainers.DevcontainerLocalFolderLabel: "/workspaces/coder",
1914+
agentcontainers.DevcontainerConfigFileLabel: "/workspaces/coder/.devcontainer/devcontainer.json",
19151915
},
19161916
}
19171917
)
@@ -1953,13 +1953,13 @@ func TestAPI(t *testing.T) {
19531953
testutil.RequireSend(ctx, t, fSAC.createErrC, nil)
19541954
testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error {
19551955
// We expect the wrong workspace agent name passed in first.
1956-
assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container")
1956+
assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=coder")
19571957
return nil
19581958
})
19591959
testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error {
19601960
// We then expect the agent name passed here to have been read from the config.
19611961
assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=custom-name")
1962-
assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container")
1962+
assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=coder")
19631963
return nil
19641964
})
19651965

0 commit comments

Comments
 (0)