Skip to content

fix: do not skip properties on creating templates #5060

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

Merged
merged 2 commits into from
Nov 14, 2022
Merged

fix: do not skip properties on creating templates #5060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7a7b81
fix: do not skip properties while creating templates
mtojek Nov 14, 2022
a9109c2
test: empty edit
mtojek Nov 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
102 changes: 100 additions & 2 deletions cli/templateedit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
func TestTemplateEdit(t *testing.T) {
t.Parallel()

t.Run("Modified", func(t *testing.T) {
t.Run("FirstEmptyThenModified", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)
Expand Down Expand Up @@ -58,7 +58,7 @@ func TestTemplateEdit(t *testing.T) {
assert.Equal(t, icon, updated.Icon)
assert.Equal(t, defaultTTL.Milliseconds(), updated.DefaultTTLMillis)
})
t.Run("NotModified", func(t *testing.T) {
t.Run("FirstEmptyThenNotModified", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)
Expand Down Expand Up @@ -124,4 +124,102 @@ func TestTemplateEdit(t *testing.T) {
assert.Equal(t, template.Name, updated.Name)
assert.Equal(t, "", template.DisplayName)
})
t.Run("WithPropertiesThenModified", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
_ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)

initialDisplayName := "This is a template"
initialDescription := "This is description"
initialIcon := "/img/icon.png"

template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
ctr.DisplayName = initialDisplayName
ctr.Description = initialDescription
ctr.Icon = initialIcon
})

// Test created template
created, err := client.Template(context.Background(), template.ID)
require.NoError(t, err)
assert.Equal(t, initialDisplayName, created.DisplayName)
assert.Equal(t, initialDescription, created.Description)
assert.Equal(t, initialIcon, created.Icon)

// Test the cli command.
displayName := "New Display Name 789"
icon := "/icons/new-icon.png"
cmdArgs := []string{
"templates",
"edit",
template.Name,
"--display-name", displayName,
"--icon", icon,
}
cmd, root := clitest.New(t, cmdArgs...)
clitest.SetupConfig(t, client, root)

ctx, _ := testutil.Context(t)
err = cmd.ExecuteContext(ctx)

require.NoError(t, err)

// Assert that the template metadata changed.
updated, err := client.Template(context.Background(), template.ID)
require.NoError(t, err)
assert.Equal(t, template.Name, updated.Name) // doesn't change
assert.Equal(t, initialDescription, updated.Description) // doesn't change
assert.Equal(t, displayName, updated.DisplayName)
assert.Equal(t, icon, updated.Icon)
})
t.Run("WithPropertiesThenEmptyEdit", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
_ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)

initialDisplayName := "This is a template"
initialDescription := "This is description"
initialIcon := "/img/icon.png"

template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
ctr.DisplayName = initialDisplayName
ctr.Description = initialDescription
ctr.Icon = initialIcon
})

// Test created template
created, err := client.Template(context.Background(), template.ID)
require.NoError(t, err)
assert.Equal(t, initialDisplayName, created.DisplayName)
assert.Equal(t, initialDescription, created.Description)
assert.Equal(t, initialIcon, created.Icon)

// Test the cli command.
cmdArgs := []string{
"templates",
"edit",
template.Name,
}
cmd, root := clitest.New(t, cmdArgs...)
clitest.SetupConfig(t, client, root)

ctx, _ := testutil.Context(t)
err = cmd.ExecuteContext(ctx)

require.NoError(t, err)

// Assert that the template metadata changed.
updated, err := client.Template(context.Background(), template.ID)
require.NoError(t, err)
// Properties don't change
assert.Equal(t, template.Name, updated.Name)
assert.Equal(t, template.Description, updated.Description)
assert.Equal(t, template.DisplayName, updated.DisplayName)
// Icon is removed, as the API considers it as "delete" request
assert.Equal(t, "", updated.Icon)
Copy link
Member Author

@mtojek mtojek Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is really unfortunate. If somebody runs coder templates edit <template-name>, they will automatically remove the icon. This is an outcome of not having a clear difference between delete icon and do not modify. Do you think that it's worth opening another issue?

EDIT:

Side issue - it isn't possible to clear the template description with UI. User can submit an empty description field, but the content is not deleted.

Copy link
Member

Choose a reason for hiding this comment

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

Considering the behavior here:

coder/coderd/templates.go

Lines 481 to 496 in 4b3d211

// Update template metadata -- empty fields are not overwritten.
name := req.Name
displayName := req.DisplayName
desc := req.Description
icon := req.Icon
maxTTL := time.Duration(req.DefaultTTLMillis) * time.Millisecond
if name == "" {
name = template.Name
}
if displayName == "" {
displayName = template.DisplayName
}
if desc == "" {
desc = template.Description
}

I would definitely consider it a bug. It's inconsistent how fields are affected. A missing field (null) should produce no change, an empty string should remove it and a non empty string change it. Feel free to open an issue about this. 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

#5066

})
}
2 changes: 2 additions & 0 deletions coderd/database/databasefake/databasefake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,6 +2091,8 @@ func (q *fakeQuerier) InsertTemplate(_ context.Context, arg database.InsertTempl
CreatedBy: arg.CreatedBy,
UserACL: arg.UserACL,
GroupACL: arg.GroupACL,
DisplayName: arg.DisplayName,
Icon: arg.Icon,
}
q.templates = append(q.templates, template)
return template, nil
Expand Down
2 changes: 2 additions & 0 deletions coderd/templates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque
GroupACL: database.TemplateACL{
organization.ID.String(): []rbac.Action{rbac.ActionRead},
},
DisplayName: createTemplate.DisplayName,
Icon: createTemplate.Icon,
})
if err != nil {
return xerrors.Errorf("insert template: %s", err)
Expand Down