Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mtojek merged 2 commits intocoder:mainfrommtojek:3321-template-space-c
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
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 deletionscli/templateedit_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 DownExpand 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
MemberAuthor

@mtojekmtojekNov 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 runscoder templates edit <template-name>, they will automatically remove the icon. This is an outcome of not having a clear difference betweendelete icon anddo 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:

// 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
ifname=="" {
name=template.Name
}
ifdisplayName=="" {
displayName=template.DisplayName
}
ifdesc=="" {
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. 👍🏻

mtojek reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

})
}
2 changes: 2 additions & 0 deletionscoderd/database/databasefake/databasefake.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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 deletionscoderd/templates.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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
[8]ページ先頭
©2009-2025 Movatter.jp