Skip to content

Commit 3ddea61

Browse files
ThomasK33ThomasK33
committed
fix: address aggregated apiserver review feedback
1 parent 981fdf5 commit 3ddea61

4 files changed

Lines changed: 267 additions & 42 deletions

File tree

‎internal/aggregated/convert/workspace.go‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,16 @@ func WorkspaceToK8s(namespace string, w codersdk.Workspace) *aggregationv1alpha1
5151
}
5252

5353
func workspaceRunning(workspace codersdk.Workspace) bool {
54-
running := workspace.LatestBuild.Transition == codersdk.WorkspaceTransitionStart
55-
if workspace.LatestBuild.Status == codersdk.WorkspaceStatusRunning {
56-
running = true
54+
if workspace.LatestBuild.Transition != codersdk.WorkspaceTransitionStart {
55+
return false
5756
}
5857

59-
return running
58+
switch workspace.LatestBuild.Status {
59+
case codersdk.WorkspaceStatusPending, codersdk.WorkspaceStatusStarting, codersdk.WorkspaceStatusRunning:
60+
return true
61+
default:
62+
return false
63+
}
6064
}
6165

6266
// WorkspaceCreateRequestFromK8s builds a codersdk.CreateWorkspaceRequest.
@@ -75,10 +79,23 @@ func WorkspaceCreateRequestFromK8s(
7579
panic("assertion failed: template ID must not be nil")
7680
}
7781

78-
return codersdk.CreateWorkspaceRequest{
82+
request := codersdk.CreateWorkspaceRequest{
7983
Name: workspaceName,
80-
TemplateID: templateID,
8184
TTLMillis: obj.Spec.TTLMillis,
8285
AutostartSchedule: obj.Spec.AutostartSchedule,
8386
}
87+
88+
if obj.Spec.TemplateVersionID == "" {
89+
request.TemplateID = templateID
90+
return request
91+
}
92+
93+
templateVersionID, err := uuid.Parse(obj.Spec.TemplateVersionID)
94+
if err != nil {
95+
request.TemplateID = templateID
96+
return request
97+
}
98+
99+
request.TemplateVersionID = templateVersionID
100+
return request
84101
}

‎internal/aggregated/convert/workspace_test.go‎

Lines changed: 119 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -89,28 +89,86 @@ func TestWorkspaceToK8s(t *testing.T) {
8989
}
9090
}
9191

92-
func TestWorkspaceToK8sInfersRunningFromBuildStatus(t *testing.T) {
92+
func TestWorkspaceToK8sRunningStateFromTransitionAndStatus(t *testing.T) {
9393
t.Parallel()
9494

95-
workspace := codersdk.Workspace{
96-
ID: uuid.New(),
97-
CreatedAt: time.Now().UTC(),
98-
UpdatedAt: time.Now().UTC(),
99-
OwnerName: "alice",
100-
OrganizationName: "acme",
101-
TemplateName: "starter-template",
102-
Name: "dev-workspace",
103-
LastUsedAt: time.Now().UTC(),
104-
LatestBuild: codersdk.WorkspaceBuild{
105-
ID: uuid.New(),
106-
Transition: codersdk.WorkspaceTransitionStop,
107-
Status: codersdk.WorkspaceStatusRunning,
95+
testCases := []struct {
96+
name string
97+
transition codersdk.WorkspaceTransition
98+
status codersdk.WorkspaceStatus
99+
running bool
100+
}{
101+
{
102+
name: "start pending",
103+
transition: codersdk.WorkspaceTransitionStart,
104+
status: codersdk.WorkspaceStatusPending,
105+
running: true,
106+
},
107+
{
108+
name: "start starting",
109+
transition: codersdk.WorkspaceTransitionStart,
110+
status: codersdk.WorkspaceStatusStarting,
111+
running: true,
112+
},
113+
{
114+
name: "start running",
115+
transition: codersdk.WorkspaceTransitionStart,
116+
status: codersdk.WorkspaceStatusRunning,
117+
running: true,
118+
},
119+
{
120+
name: "start failed",
121+
transition: codersdk.WorkspaceTransitionStart,
122+
status: codersdk.WorkspaceStatusFailed,
123+
running: false,
124+
},
125+
{
126+
name: "start canceled",
127+
transition: codersdk.WorkspaceTransitionStart,
128+
status: codersdk.WorkspaceStatusCanceled,
129+
running: false,
130+
},
131+
{
132+
name: "stop running",
133+
transition: codersdk.WorkspaceTransitionStop,
134+
status: codersdk.WorkspaceStatusRunning,
135+
running: false,
108136
},
109137
}
110138

111-
converted := WorkspaceToK8s("control-plane", workspace)
112-
if !converted.Spec.Running {
113-
t.Fatal("expected running=true when latest build status is running")
139+
for _, testCase := range testCases {
140+
testCase := testCase
141+
t.Run(testCase.name, func(t *testing.T) {
142+
t.Parallel()
143+
144+
now := time.Date(2025, time.February, 2, 3, 4, 5, 0, time.UTC)
145+
workspace := codersdk.Workspace{
146+
ID: uuid.New(),
147+
CreatedAt: now,
148+
UpdatedAt: now,
149+
OwnerName: "alice",
150+
OrganizationName: "acme",
151+
TemplateName: "starter-template",
152+
Name: "dev-workspace",
153+
LastUsedAt: now,
154+
LatestBuild: codersdk.WorkspaceBuild{
155+
ID: uuid.New(),
156+
Transition: testCase.transition,
157+
Status: testCase.status,
158+
},
159+
}
160+
161+
converted := WorkspaceToK8s("control-plane", workspace)
162+
if converted.Spec.Running != testCase.running {
163+
t.Fatalf(
164+
"expected running=%t for transition=%q status=%q, got %t",
165+
testCase.running,
166+
testCase.transition,
167+
testCase.status,
168+
converted.Spec.Running,
169+
)
170+
}
171+
})
114172
}
115173
}
116174

@@ -135,10 +193,54 @@ func TestWorkspaceCreateRequestFromK8s(t *testing.T) {
135193
if request.TemplateID != templateID {
136194
t.Fatalf("expected request template ID %q, got %q", templateID, request.TemplateID)
137195
}
196+
if request.TemplateVersionID != uuid.Nil {
197+
t.Fatalf("expected request template version ID %q, got %q", uuid.Nil, request.TemplateVersionID)
198+
}
138199
if request.TTLMillis == nil || *request.TTLMillis != ttlMillis {
139200
t.Fatalf("expected request TTL millis %d, got %+v", ttlMillis, request.TTLMillis)
140201
}
141202
if request.AutostartSchedule == nil || *request.AutostartSchedule != autostartSchedule {
142203
t.Fatalf("expected request autostart schedule %q, got %+v", autostartSchedule, request.AutostartSchedule)
143204
}
144205
}
206+
207+
func TestWorkspaceCreateRequestFromK8sUsesTemplateVersionID(t *testing.T) {
208+
t.Parallel()
209+
210+
templateID := uuid.New()
211+
templateVersionID := uuid.New()
212+
213+
obj := &aggregationv1alpha1.CoderWorkspace{
214+
Spec: aggregationv1alpha1.CoderWorkspaceSpec{
215+
TemplateVersionID: templateVersionID.String(),
216+
},
217+
}
218+
219+
request := WorkspaceCreateRequestFromK8s(obj, "dev-workspace", templateID)
220+
if request.TemplateVersionID != templateVersionID {
221+
t.Fatalf("expected request template version ID %q, got %q", templateVersionID, request.TemplateVersionID)
222+
}
223+
if request.TemplateID != uuid.Nil {
224+
t.Fatalf("expected request template ID %q, got %q", uuid.Nil, request.TemplateID)
225+
}
226+
}
227+
228+
func TestWorkspaceCreateRequestFromK8sFallsBackToTemplateIDForInvalidTemplateVersionID(t *testing.T) {
229+
t.Parallel()
230+
231+
templateID := uuid.New()
232+
233+
obj := &aggregationv1alpha1.CoderWorkspace{
234+
Spec: aggregationv1alpha1.CoderWorkspaceSpec{
235+
TemplateVersionID: "not-a-uuid",
236+
},
237+
}
238+
239+
request := WorkspaceCreateRequestFromK8s(obj, "dev-workspace", templateID)
240+
if request.TemplateID != templateID {
241+
t.Fatalf("expected request template ID %q, got %q", templateID, request.TemplateID)
242+
}
243+
if request.TemplateVersionID != uuid.Nil {
244+
t.Fatalf("expected request template version ID %q, got %q", uuid.Nil, request.TemplateVersionID)
245+
}
246+
}

‎internal/app/apiserverapp/apiserverapp.go‎

Lines changed: 75 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"net"
88
"net/url"
9+
"strings"
910
"time"
1011

1112
metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
@@ -28,6 +29,7 @@ import (
2829
aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1"
2930
"github.com/coder/coder-k8s/internal/aggregated/coder"
3031
"github.com/coder/coder-k8s/internal/aggregated/storage"
32+
"github.com/coder/coder/v2/codersdk"
3133
)
3234

3335
const (
@@ -50,6 +52,78 @@ type Options struct {
5052
CoderRequestTimeout time.Duration
5153
}
5254

55+
type errClientProvider struct {
56+
err error
57+
}
58+
59+
var _ coder.ClientProvider = (*errClientProvider)(nil)
60+
61+
func (p *errClientProvider) ClientForNamespace(ctx context.Context, namespace string) (*codersdk.Client, error) {
62+
if p == nil {
63+
return nil, fmt.Errorf("assertion failed: error client provider must not be nil")
64+
}
65+
if ctx == nil {
66+
return nil, fmt.Errorf("assertion failed: context must not be nil")
67+
}
68+
if namespace == "" {
69+
return nil, fmt.Errorf("assertion failed: namespace must not be empty")
70+
}
71+
if p.err == nil {
72+
return nil, fmt.Errorf("assertion failed: error client provider error must not be nil")
73+
}
74+
75+
return nil, p.err
76+
}
77+
78+
func buildClientProvider(opts Options, requestTimeout time.Duration) (coder.ClientProvider, error) {
79+
if requestTimeout <= 0 {
80+
return nil, fmt.Errorf("assertion failed: request timeout must be positive")
81+
}
82+
83+
coderURL := strings.TrimSpace(opts.CoderURL)
84+
sessionToken := strings.TrimSpace(opts.CoderSessionToken)
85+
missing := make([]string, 0, 2)
86+
if coderURL == "" {
87+
missing = append(missing, "coder URL")
88+
}
89+
if sessionToken == "" {
90+
missing = append(missing, "coder session token")
91+
}
92+
if len(missing) > 0 {
93+
provider := &errClientProvider{err: fmt.Errorf(
94+
"coder client provider is not configured: missing %s; configure --coder-url and --coder-session-token to enable coderworkspaces and codertemplates operations",
95+
strings.Join(missing, " and "),
96+
)}
97+
if provider.err == nil {
98+
return nil, fmt.Errorf("assertion failed: fallback error client provider error is nil after successful construction")
99+
}
100+
101+
return provider, nil
102+
}
103+
104+
parsedCoderURL, err := url.Parse(coderURL)
105+
if err != nil {
106+
return nil, fmt.Errorf("parse coder URL %q: %w", coderURL, err)
107+
}
108+
if parsedCoderURL == nil {
109+
return nil, fmt.Errorf("assertion failed: parsed coder URL must not be nil")
110+
}
111+
112+
provider, err := coder.NewStaticClientProvider(coder.Config{
113+
CoderURL: parsedCoderURL,
114+
SessionToken: sessionToken,
115+
RequestTimeout: requestTimeout,
116+
})
117+
if err != nil {
118+
return nil, err
119+
}
120+
if provider == nil {
121+
return nil, fmt.Errorf("assertion failed: coder client provider is nil after successful construction")
122+
}
123+
124+
return provider, nil
125+
}
126+
53127
// NewScheme builds the runtime scheme used by the aggregated API server.
54128
func NewScheme() *runtime.Scheme {
55129
scheme := runtime.NewScheme()
@@ -179,34 +253,16 @@ func RunWithOptions(ctx context.Context, opts Options) error {
179253
if ctx == nil {
180254
return fmt.Errorf("assertion failed: context must not be nil")
181255
}
182-
if opts.CoderURL == "" {
183-
return fmt.Errorf("assertion failed: coder URL must not be empty")
184-
}
185-
if opts.CoderSessionToken == "" {
186-
return fmt.Errorf("assertion failed: coder session token must not be empty")
187-
}
188256
if opts.CoderRequestTimeout < 0 {
189257
return fmt.Errorf("assertion failed: coder request timeout must not be negative")
190258
}
191259

192-
parsedCoderURL, err := url.Parse(opts.CoderURL)
193-
if err != nil {
194-
return fmt.Errorf("parse coder URL %q: %w", opts.CoderURL, err)
195-
}
196-
if parsedCoderURL == nil {
197-
return fmt.Errorf("assertion failed: parsed coder URL must not be nil")
198-
}
199-
200260
requestTimeout := opts.CoderRequestTimeout
201261
if requestTimeout == 0 {
202262
requestTimeout = 30 * time.Second
203263
}
204264

205-
provider, err := coder.NewStaticClientProvider(coder.Config{
206-
CoderURL: parsedCoderURL,
207-
SessionToken: opts.CoderSessionToken,
208-
RequestTimeout: requestTimeout,
209-
})
265+
provider, err := buildClientProvider(opts, requestTimeout)
210266
if err != nil {
211267
return fmt.Errorf("build coder client provider: %w", err)
212268
}

‎internal/app/apiserverapp/apiserverapp_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ import (
55
"net"
66
"net/http/httptest"
77
"net/url"
8+
"strings"
89
"testing"
10+
"time"
911

1012
"k8s.io/apimachinery/pkg/runtime/schema"
1113
"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -120,3 +122,51 @@ func TestInstallAPIGroupRegistersDiscovery(t *testing.T) {
120122
t.Fatalf("expected discovery registration for group %s", aggregationv1alpha1.SchemeGroupVersion.Group)
121123
}
122124
}
125+
126+
func TestBuildClientProviderReturnsDeferredErrorWithoutCoderConfig(t *testing.T) {
127+
t.Parallel()
128+
129+
provider, err := buildClientProvider(Options{}, 30*time.Second)
130+
if err != nil {
131+
t.Fatalf("build client provider: %v", err)
132+
}
133+
if provider == nil {
134+
t.Fatal("expected non-nil provider")
135+
}
136+
137+
_, err = provider.ClientForNamespace(context.Background(), "control-plane")
138+
if err == nil {
139+
t.Fatal("expected deferred client error when coder config is missing")
140+
}
141+
if !strings.Contains(err.Error(), "missing coder URL and coder session token") {
142+
t.Fatalf("expected missing-config error, got %q", err)
143+
}
144+
}
145+
146+
func TestBuildClientProviderReturnsStaticProviderWithCoderConfig(t *testing.T) {
147+
t.Parallel()
148+
149+
provider, err := buildClientProvider(Options{
150+
CoderURL: "https://coder.example.com",
151+
CoderSessionToken: "test-session-token",
152+
}, 30*time.Second)
153+
if err != nil {
154+
t.Fatalf("build client provider: %v", err)
155+
}
156+
157+
staticProvider, ok := provider.(*coderhelper.StaticClientProvider)
158+
if !ok {
159+
t.Fatalf("expected *coder.StaticClientProvider, got %T", provider)
160+
}
161+
162+
sdkClient, err := staticProvider.ClientForNamespace(context.Background(), "control-plane")
163+
if err != nil {
164+
t.Fatalf("resolve static client for namespace: %v", err)
165+
}
166+
if sdkClient == nil {
167+
t.Fatal("expected non-nil sdk client")
168+
}
169+
if got, want := sdkClient.URL.String(), "https://coder.example.com"; got != want {
170+
t.Fatalf("expected client URL %q, got %q", want, got)
171+
}
172+
}

0 commit comments

Comments
 (0)