Skip to content

Commit aecdafd

Browse files
sreyasreya
authored
fix: fix template edit overriding with flag defaults (#11564)
1 parent eb8d85f commit aecdafd

File tree

7 files changed

+238
-28
lines changed

7 files changed

+238
-28
lines changed

‎cli/templateedit.go

+61-23
Original file line numberDiff line numberDiff line change
@@ -87,48 +87,86 @@ func (r *RootCmd) templateEdit() *clibase.Cmd {
8787
return xerrors.Errorf("get workspace template: %w", err)
8888
}
8989

90-
// Copy the default value if the list is empty, or if the user
91-
// specified the "none" value clear the list.
92-
if len(autostopRequirementDaysOfWeek) == 0 {
93-
autostopRequirementDaysOfWeek = template.AutostopRequirement.DaysOfWeek
90+
// Default values
91+
if !userSetOption(inv, "description") {
92+
description = template.Description
9493
}
95-
if len(autostartRequirementDaysOfWeek) == 1 && autostartRequirementDaysOfWeek[0] == "all" {
96-
// Set it to every day of the week
97-
autostartRequirementDaysOfWeek = []string{"monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"}
98-
} else if len(autostartRequirementDaysOfWeek) == 0 {
99-
autostartRequirementDaysOfWeek = template.AutostartRequirement.DaysOfWeek
94+
95+
if !userSetOption(inv, "icon") {
96+
icon = template.Icon
10097
}
101-
if unsetAutostopRequirementDaysOfWeek {
102-
autostopRequirementDaysOfWeek = []string{}
98+
99+
if !userSetOption(inv, "display-name") {
100+
displayName = template.DisplayName
103101
}
104-
if failureTTL == 0 {
102+
103+
if !userSetOption(inv, "max-ttl") {
104+
maxTTL = time.Duration(template.MaxTTLMillis) * time.Millisecond
105+
}
106+
107+
if !userSetOption(inv, "default-ttl") {
108+
defaultTTL = time.Duration(template.DefaultTTLMillis) * time.Millisecond
109+
}
110+
111+
if !userSetOption(inv, "allow-user-autostop") {
112+
allowUserAutostop = template.AllowUserAutostop
113+
}
114+
115+
if !userSetOption(inv, "allow-user-autostart") {
116+
allowUserAutostart = template.AllowUserAutostart
117+
}
118+
119+
if !userSetOption(inv, "allow-user-cancel-workspace-jobs") {
120+
allowUserCancelWorkspaceJobs = template.AllowUserCancelWorkspaceJobs
121+
}
122+
123+
if !userSetOption(inv, "failure-ttl") {
105124
failureTTL = time.Duration(template.FailureTTLMillis) * time.Millisecond
106125
}
107-
if dormancyThreshold == 0 {
126+
127+
if !userSetOption(inv, "dormancy-threshold") {
108128
dormancyThreshold = time.Duration(template.TimeTilDormantMillis) * time.Millisecond
109129
}
110-
if dormancyAutoDeletion == 0 {
130+
131+
if !userSetOption(inv, "dormancy-auto-deletion") {
111132
dormancyAutoDeletion = time.Duration(template.TimeTilDormantAutoDeleteMillis) * time.Millisecond
112133
}
113134

114-
// Default values
115-
if !userSetOption(inv, "description") {
116-
description = template.Description
135+
if !userSetOption(inv, "require-active-version") {
136+
requireActiveVersion = template.RequireActiveVersion
117137
}
118138

119-
if !userSetOption(inv, "icon") {
120-
icon = template.Icon
139+
if !userSetOption(inv, "autostop-requirement-weekdays") {
140+
autostopRequirementDaysOfWeek = template.AutostopRequirement.DaysOfWeek
121141
}
122142

123-
if !userSetOption(inv, "display-name") {
124-
displayName = template.DisplayName
143+
if unsetAutostopRequirementDaysOfWeek {
144+
autostopRequirementDaysOfWeek = []string{}
145+
}
146+
147+
if !userSetOption(inv, "autostop-requirement-weeks") {
148+
autostopRequirementWeeks = template.AutostopRequirement.Weeks
149+
}
150+
151+
if len(autostartRequirementDaysOfWeek) == 1 && autostartRequirementDaysOfWeek[0] == "all" {
152+
// Set it to every day of the week
153+
autostartRequirementDaysOfWeek = []string{"monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"}
154+
} else if !userSetOption(inv, "autostart-requirement-weekdays") {
155+
autostartRequirementDaysOfWeek = template.AutostartRequirement.DaysOfWeek
156+
} else if len(autostartRequirementDaysOfWeek) == 0 {
157+
autostartRequirementDaysOfWeek = []string{}
125158
}
126159

127160
var deprecated *string
128-
if !userSetOption(inv, "deprecated") {
161+
if userSetOption(inv, "deprecated") {
129162
deprecated = &deprecationMessage
130163
}
131164

165+
var disableEveryoneGroup bool
166+
if userSetOption(inv, "private") {
167+
disableEveryoneGroup = disableEveryone
168+
}
169+
132170
req := codersdk.UpdateTemplateMeta{
133171
Name: name,
134172
DisplayName: displayName,
@@ -151,7 +189,7 @@ func (r *RootCmd) templateEdit() *clibase.Cmd {
151189
AllowUserAutostop: allowUserAutostop,
152190
RequireActiveVersion: requireActiveVersion,
153191
DeprecationMessage: deprecated,
154-
DisableEveryoneGroupAccess: disableEveryone,
192+
DisableEveryoneGroupAccess: disableEveryoneGroup,
155193
}
156194

157195
_, err = client.UpdateTemplateMeta(inv.Context(), template.ID, req)

‎coderd/database/dbgen/dbgen.go

+3
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ func Template(t testing.TB, db database.Store, seed database.Template) database.
7272
seed.OrganizationID.String(): []rbac.Action{rbac.ActionRead},
7373
}
7474
}
75+
if seed.UserACL == nil {
76+
seed.UserACL = database.TemplateACL{}
77+
}
7578
err := db.InsertTemplate(genCtx, database.InsertTemplateParams{
7679
ID: id,
7780
CreatedAt: takeFirst(seed.CreatedAt, dbtime.Now()),

‎coderd/database/dbmem/dbmem.go

+1
Original file line numberDiff line numberDiff line change
@@ -6374,6 +6374,7 @@ func (q *FakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd
63746374
tpl.Description = arg.Description
63756375
tpl.Icon = arg.Icon
63766376
tpl.GroupACL = arg.GroupACL
6377+
tpl.AllowUserCancelWorkspaceJobs = arg.AllowUserCancelWorkspaceJobs
63776378
q.templates[idx] = tpl
63786379
return nil
63796380
}

‎coderd/externalauth/externalauth.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut
325325
// return a better error.
326326
switch resp.StatusCode {
327327
case http.StatusTooManyRequests:
328-
return nil, fmt.Errorf("rate limit hit, unable to authorize device. please try again later")
328+
return nil, xerrors.New("rate limit hit, unable to authorize device. please try again later")
329329
default:
330330
return nil, err
331331
}

‎coderd/promoauth/github.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
package promoauth
22

33
import (
4-
"fmt"
54
"net/http"
65
"strconv"
76
"time"
7+
8+
"golang.org/x/xerrors"
89
)
910

1011
type rateLimits struct {
@@ -81,7 +82,7 @@ func (p *headerParser) string(key string) string {
8182

8283
v := p.header.Get(key)
8384
if v == "" {
84-
p.errors[key] = fmt.Errorf("missing header %q", key)
85+
p.errors[key] = xerrors.Errorf("missing header %q", key)
8586
}
8687
return v
8788
}

‎coderd/templates.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,7 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
669669

670670
groupACL := template.GroupACL
671671
if req.DisableEveryoneGroupAccess {
672-
groupACL = database.TemplateACL{}
672+
delete(groupACL, template.OrganizationID.String())
673673
}
674674

675675
var err error

‎enterprise/cli/templateedit_test.go

+168-1
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,15 @@ import (
44
"testing"
55
"time"
66

7+
"github.com/stretchr/testify/assert"
78
"github.com/stretchr/testify/require"
89

910
"github.com/coder/coder/v2/cli/clitest"
1011
"github.com/coder/coder/v2/coderd/coderdtest"
12+
"github.com/coder/coder/v2/coderd/database"
13+
"github.com/coder/coder/v2/coderd/database/dbfake"
1114
"github.com/coder/coder/v2/coderd/rbac"
15+
"github.com/coder/coder/v2/coderd/util/ptr"
1216
"github.com/coder/coder/v2/codersdk"
1317
"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
1418
"github.com/coder/coder/v2/enterprise/coderd/license"
@@ -105,7 +109,6 @@ func TestTemplateEdit(t *testing.T) {
105109
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID)
106110
template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID)
107111
require.False(t, template.RequireActiveVersion)
108-
109112
const (
110113
expectedFailureTTL = time.Hour * 3
111114
expectedDormancyThreshold = time.Hour * 4
@@ -150,4 +153,168 @@ func TestTemplateEdit(t *testing.T) {
150153
require.Equal(t, expectedDormancyThreshold.Milliseconds(), template.TimeTilDormantMillis)
151154
require.Equal(t, expectedDormancyAutoDeletion.Milliseconds(), template.TimeTilDormantAutoDeleteMillis)
152155
})
156+
157+
// Test that omitting a flag does not update a template with the
158+
// default for a flag.
159+
t.Run("DefaultsDontOverride", func(t *testing.T) {
160+
t.Parallel()
161+
162+
ctx := testutil.Context(t, testutil.WaitMedium)
163+
ownerClient, db, owner := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{
164+
LicenseOptions: &coderdenttest.LicenseOptions{
165+
Features: license.Features{
166+
codersdk.FeatureAdvancedTemplateScheduling: 1,
167+
codersdk.FeatureAccessControl: 1,
168+
codersdk.FeatureTemplateRBAC: 1,
169+
},
170+
},
171+
})
172+
173+
dbtemplate := dbfake.TemplateVersion(t, db).Seed(database.TemplateVersion{
174+
CreatedBy: owner.UserID,
175+
OrganizationID: owner.OrganizationID,
176+
}).Do().Template
177+
178+
var (
179+
expectedName = "template"
180+
expectedDisplayName = "template_display"
181+
expectedDescription = "My description"
182+
expectedIcon = "icon.pjg"
183+
expectedDefaultTTLMillis = time.Hour.Milliseconds()
184+
expectedMaxTTLMillis = (time.Hour * 24).Milliseconds()
185+
expectedAllowAutostart = false
186+
expectedAllowAutostop = false
187+
expectedFailureTTLMillis = time.Minute.Milliseconds()
188+
expectedDormancyMillis = 2 * time.Minute.Milliseconds()
189+
expectedAutoDeleteMillis = 3 * time.Minute.Milliseconds()
190+
expectedRequireActiveVersion = true
191+
expectedAllowCancelJobs = false
192+
deprecationMessage = "Deprecate me"
193+
expectedDisableEveryone = true
194+
expectedAutostartDaysOfWeek = []string{}
195+
expectedAutoStopDaysOfWeek = []string{}
196+
expectedAutoStopWeeks = 1
197+
)
198+
199+
assertFieldsFn := func(t *testing.T, tpl codersdk.Template, acl codersdk.TemplateACL) {
200+
t.Helper()
201+
202+
assert.Equal(t, expectedName, tpl.Name)
203+
assert.Equal(t, expectedDisplayName, tpl.DisplayName)
204+
assert.Equal(t, expectedDescription, tpl.Description)
205+
assert.Equal(t, expectedIcon, tpl.Icon)
206+
assert.Equal(t, expectedDefaultTTLMillis, tpl.DefaultTTLMillis)
207+
assert.Equal(t, expectedMaxTTLMillis, tpl.MaxTTLMillis)
208+
assert.Equal(t, expectedAllowAutostart, tpl.AllowUserAutostart)
209+
assert.Equal(t, expectedAllowAutostop, tpl.AllowUserAutostop)
210+
assert.Equal(t, expectedFailureTTLMillis, tpl.FailureTTLMillis)
211+
assert.Equal(t, expectedDormancyMillis, tpl.TimeTilDormantMillis)
212+
assert.Equal(t, expectedAutoDeleteMillis, tpl.TimeTilDormantAutoDeleteMillis)
213+
assert.Equal(t, expectedRequireActiveVersion, tpl.RequireActiveVersion)
214+
assert.Equal(t, deprecationMessage, tpl.DeprecationMessage)
215+
assert.Equal(t, expectedAllowCancelJobs, tpl.AllowUserCancelWorkspaceJobs)
216+
assert.Equal(t, len(acl.Groups) == 0, expectedDisableEveryone)
217+
assert.Equal(t, expectedAutostartDaysOfWeek, tpl.AutostartRequirement.DaysOfWeek)
218+
assert.Equal(t, expectedAutoStopDaysOfWeek, tpl.AutostopRequirement.DaysOfWeek)
219+
assert.Equal(t, int64(expectedAutoStopWeeks), tpl.AutostopRequirement.Weeks)
220+
}
221+
222+
template, err := ownerClient.UpdateTemplateMeta(ctx, dbtemplate.ID, codersdk.UpdateTemplateMeta{
223+
Name: expectedName,
224+
DisplayName: expectedDisplayName,
225+
Description: expectedDescription,
226+
Icon: expectedIcon,
227+
DefaultTTLMillis: expectedDefaultTTLMillis,
228+
MaxTTLMillis: expectedMaxTTLMillis,
229+
AllowUserAutostop: expectedAllowAutostop,
230+
AllowUserAutostart: expectedAllowAutostart,
231+
FailureTTLMillis: expectedFailureTTLMillis,
232+
TimeTilDormantMillis: expectedDormancyMillis,
233+
TimeTilDormantAutoDeleteMillis: expectedAutoDeleteMillis,
234+
RequireActiveVersion: expectedRequireActiveVersion,
235+
DeprecationMessage: ptr.Ref(deprecationMessage),
236+
DisableEveryoneGroupAccess: expectedDisableEveryone,
237+
AllowUserCancelWorkspaceJobs: expectedAllowCancelJobs,
238+
AutostartRequirement: &codersdk.TemplateAutostartRequirement{
239+
DaysOfWeek: expectedAutostartDaysOfWeek,
240+
},
241+
})
242+
require.NoError(t, err)
243+
244+
templateACL, err := ownerClient.TemplateACL(ctx, template.ID)
245+
require.NoError(t, err)
246+
247+
assertFieldsFn(t, template, templateACL)
248+
249+
expectedName = "newName"
250+
inv, conf := newCLI(t, "templates",
251+
"edit", template.Name,
252+
"--name=newName",
253+
"-y",
254+
)
255+
256+
clitest.SetupConfig(t, ownerClient, conf)
257+
258+
err = inv.Run()
259+
require.NoError(t, err)
260+
261+
template, err = ownerClient.Template(ctx, template.ID)
262+
require.NoError(t, err)
263+
templateACL, err = ownerClient.TemplateACL(ctx, template.ID)
264+
require.NoError(t, err)
265+
assertFieldsFn(t, template, templateACL)
266+
267+
expectedAutostartDaysOfWeek = []string{"monday", "wednesday", "friday"}
268+
expectedAutoStopDaysOfWeek = []string{"tuesday", "thursday"}
269+
expectedAutoStopWeeks = 2
270+
expectedMaxTTLMillis = 0
271+
272+
template, err = ownerClient.UpdateTemplateMeta(ctx, dbtemplate.ID, codersdk.UpdateTemplateMeta{
273+
Name: expectedName,
274+
DisplayName: expectedDisplayName,
275+
Description: expectedDescription,
276+
Icon: expectedIcon,
277+
DefaultTTLMillis: expectedDefaultTTLMillis,
278+
AllowUserAutostop: expectedAllowAutostop,
279+
AllowUserAutostart: expectedAllowAutostart,
280+
FailureTTLMillis: expectedFailureTTLMillis,
281+
TimeTilDormantMillis: expectedDormancyMillis,
282+
TimeTilDormantAutoDeleteMillis: expectedAutoDeleteMillis,
283+
RequireActiveVersion: expectedRequireActiveVersion,
284+
DeprecationMessage: ptr.Ref(deprecationMessage),
285+
DisableEveryoneGroupAccess: expectedDisableEveryone,
286+
AllowUserCancelWorkspaceJobs: expectedAllowCancelJobs,
287+
AutostartRequirement: &codersdk.TemplateAutostartRequirement{
288+
DaysOfWeek: expectedAutostartDaysOfWeek,
289+
},
290+
291+
AutostopRequirement: &codersdk.TemplateAutostopRequirement{
292+
DaysOfWeek: expectedAutoStopDaysOfWeek,
293+
Weeks: int64(expectedAutoStopWeeks),
294+
},
295+
})
296+
require.NoError(t, err)
297+
assertFieldsFn(t, template, templateACL)
298+
299+
// Rerun the update so we can assert that autostop days aren't
300+
// mucked with.
301+
expectedName = "newName2"
302+
inv, conf = newCLI(t, "templates",
303+
"edit", template.Name,
304+
"--name=newName2",
305+
"-y",
306+
)
307+
308+
clitest.SetupConfig(t, ownerClient, conf)
309+
310+
err = inv.Run()
311+
require.NoError(t, err)
312+
313+
template, err = ownerClient.Template(ctx, template.ID)
314+
require.NoError(t, err)
315+
316+
templateACL, err = ownerClient.TemplateACL(ctx, template.ID)
317+
require.NoError(t, err)
318+
assertFieldsFn(t, template, templateACL)
319+
})
153320
}

0 commit comments

Comments
 (0)