Skip to content

feat: cli: allow editing template metadata #2159

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 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 4 commits into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
61 changes: 61 additions & 0 deletions cli/templateedit.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package cli

import (
"fmt"
"time"

"github.com/spf13/cobra"
"golang.org/x/xerrors"

"github.com/coder/coder/cli/cliui"
"github.com/coder/coder/codersdk"
)

func templateEdit() *cobra.Command {
var (
description string
maxTTL time.Duration
minAutostartInterval time.Duration
)

cmd := &cobra.Command{
Use: "edit <template> [flags]",
Args: cobra.ExactArgs(1),
Short: "Edit the metadata of a template by name.",
RunE: func(cmd *cobra.Command, args []string) error {
client, err := createClient(cmd)
if err != nil {
return xerrors.Errorf("create client: %w", err)
}
organization, err := currentOrganization(cmd, client)
if err != nil {
return xerrors.Errorf("get current organization: %w", err)
}
template, err := client.TemplateByName(cmd.Context(), organization.ID, args[0])
if err != nil {
return xerrors.Errorf("get workspace template: %w", err)
}

// NOTE: coderd will ignore empty fields.
req := codersdk.UpdateTemplateMeta{
Description: description,
MaxTTLMillis: maxTTL.Milliseconds(),
MinAutostartIntervalMillis: minAutostartInterval.Milliseconds(),
}

_, err = client.UpdateTemplateMeta(cmd.Context(), template.ID, req)
if err != nil {
return xerrors.Errorf("update template metadata: %w", err)
}
_, _ = fmt.Printf("Updated template metadata!\n")
return nil
},
}

cmd.Flags().StringVarP(&description, "description", "", "", "Edit the template description")
cmd.Flags().DurationVarP(&maxTTL, "max_ttl", "", 0, "Edit the template maximum time before shutdown")
cmd.Flags().DurationVarP(&minAutostartInterval, "min_autostart_interval", "", 0, "Edit the template minimum autostart interval")
cliui.AllowSkipPrompt(cmd)

return cmd
}
94 changes: 94 additions & 0 deletions cli/templateedit_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package cli_test

import (
"context"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/coderd/util/ptr"
"github.com/coder/coder/codersdk"
)

func TestTemplateEdit(t *testing.T) {
t.Parallel()

t.Run("Modified", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
_ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
ctr.Description = "original description"
ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds())
ctr.MinAutostartIntervalMillis = ptr.Ref(time.Hour.Milliseconds())
})

// Test the cli command.
desc := "lorem ipsum dolor sit amet et cetera"
maxTTL := 12 * time.Hour
minAutostartInterval := time.Minute
cmdArgs := []string{
"templates",
"edit",
template.Name,
"--description", desc,
"--max_ttl", maxTTL.String(),
"--min_autostart_interval", minAutostartInterval.String(),
}
cmd, root := clitest.New(t, cmdArgs...)
clitest.SetupConfig(t, client, root)

err := cmd.Execute()

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, desc, updated.Description)
assert.Equal(t, maxTTL.Milliseconds(), updated.MaxTTLMillis)
assert.Equal(t, minAutostartInterval.Milliseconds(), updated.MinAutostartIntervalMillis)
})

t.Run("NotModified", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
_ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
ctr.Description = "original description"
ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds())
ctr.MinAutostartIntervalMillis = ptr.Ref(time.Hour.Milliseconds())
})

// Test the cli command.
cmdArgs := []string{
"templates",
"edit",
template.Name,
"--description", template.Description,
"--max_ttl", (time.Duration(template.MaxTTLMillis) * time.Millisecond).String(),
"--min_autostart_interval", (time.Duration(template.MinAutostartIntervalMillis) * time.Millisecond).String(),
}
cmd, root := clitest.New(t, cmdArgs...)
clitest.SetupConfig(t, client, root)

err := cmd.Execute()

require.ErrorContains(t, err, "not modified")

// Assert that the template metadata did not change.
updated, err := client.Template(context.Background(), template.ID)
require.NoError(t, err)
assert.Equal(t, template.Description, updated.Description)
assert.Equal(t, template.MaxTTLMillis, updated.MaxTTLMillis)
assert.Equal(t, template.MinAutostartIntervalMillis, updated.MinAutostartIntervalMillis)
})
}
1 change: 1 addition & 0 deletions cli/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ func templates() *cobra.Command {
}
cmd.AddCommand(
templateCreate(),
templateEdit(),
templateInit(),
templateList(),
templatePlan(),
Expand Down
1 change: 1 addition & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ func New(options *Options) *API {

r.Get("/", api.template)
r.Delete("/", api.deleteTemplate)
r.Patch("/", api.patchTemplateMeta)
r.Route("/versions", func(r chi.Router) {
r.Get("/", api.templateVersionsByTemplate)
r.Patch("/", api.patchActiveTemplateVersion)
Expand Down
19 changes: 19 additions & 0 deletions coderd/database/databasefake/databasefake.go
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,25 @@ func (q *fakeQuerier) GetTemplateByOrganizationAndName(_ context.Context, arg da
return database.Template{}, sql.ErrNoRows
}

func (q *fakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.UpdateTemplateMetaByIDParams) error {
q.mutex.RLock()
defer q.mutex.RUnlock()

for idx, tpl := range q.templates {
if tpl.ID != arg.ID {
continue
}
tpl.UpdatedAt = database.Now()
tpl.Description = arg.Description
tpl.MaxTtl = arg.MaxTtl
tpl.MinAutostartInterval = arg.MinAutostartInterval
q.templates[idx] = tpl
return nil
}

return sql.ErrNoRows
}

func (q *fakeQuerier) GetTemplateVersionsByTemplateID(_ context.Context, arg database.GetTemplateVersionsByTemplateIDParams) (version []database.TemplateVersion, err error) {
q.mutex.RLock()
defer q.mutex.RUnlock()
Expand Down
1 change: 1 addition & 0 deletions coderd/database/querier.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 33 additions & 0 deletions coderd/database/queries.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions coderd/database/queries/templates.sql
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,16 @@ SET
deleted = $2
WHERE
id = $1;

-- name: UpdateTemplateMetaByID :exec
UPDATE
templates
SET
updated_at = $2,
description = $3,
max_ttl = $4,
min_autostart_interval = $5
WHERE
id = $1
RETURNING
*;
96 changes: 96 additions & 0 deletions coderd/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,102 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
httpapi.Write(rw, http.StatusOK, convertTemplate(template, count))
}

func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
template := httpmw.TemplateParam(r)
if !api.Authorize(rw, r, rbac.ActionUpdate, template) {
return
}

var req codersdk.UpdateTemplateMeta
if !httpapi.Read(rw, r, &req) {
return
}

var validErrs []httpapi.Error
if req.MaxTTLMillis < 0 {
validErrs = append(validErrs, httpapi.Error{Field: "max_ttl_ms", Detail: "Must be a positive integer."})
}
if req.MinAutostartIntervalMillis < 0 {
validErrs = append(validErrs, httpapi.Error{Field: "min_autostart_interval_ms", Detail: "Must be a positive integer."})
}

if len(validErrs) > 0 {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: "Invalid request to update template metadata!",
Validations: validErrs,
})
return
}

count := uint32(0)
var updated database.Template
err := api.Database.InTx(func(s database.Store) error {
// Fetch workspace counts
workspaceCounts, err := s.GetWorkspaceOwnerCountsByTemplateIDs(r.Context(), []uuid.UUID{template.ID})
if xerrors.Is(err, sql.ErrNoRows) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, should we add some linting for this? The actual xerrors.Is function is marked deprecated, and we have a lot of mixed usage in the code-base:

❯ rg xerrors.Is | wc -l
20

❯ rg '[^x]errors.Is' | wc -l
104

(Note: I'm not suggesting we fix the usage in this PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK there are Reasons for using that instead of the original; something about stacktraces.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there was a case for using Errorf and New at least, to get the stack trace. But the others are deprecated and are likely not updated in case there are any changes to stdlib. Interestingly, I now noticed Errorf was also marked deprecated in favor of fmt.Errorf even though the latter does not include a stack trace. Weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, might be a good topic for dicussion!

Copy link
Member

@Emyrk Emyrk Jun 8, 2022

Choose a reason for hiding this comment

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

@mafredri we can add a linter for it easy if we want to use xerrors.As/Is over errors.As/Is

	m.Match("errors.$f($*_)").
		Report("Use xerrors.$f to provide additional stacktrace information!")

err = nil
}
if err != nil {
return err
}

if len(workspaceCounts) > 0 {
count = uint32(workspaceCounts[0].Count)
}

if req.Description == template.Description &&
req.MaxTTLMillis == time.Duration(template.MaxTtl).Milliseconds() &&
req.MinAutostartIntervalMillis == time.Duration(template.MinAutostartInterval).Milliseconds() {
return nil
}

// Update template metadata -- empty fields are not overwritten.
desc := req.Description
maxTTL := time.Duration(req.MaxTTLMillis) * time.Millisecond
minAutostartInterval := time.Duration(req.MinAutostartIntervalMillis) * time.Millisecond

if desc == "" {
desc = template.Description
}
if maxTTL == 0 {
maxTTL = time.Duration(template.MaxTtl)
}
if minAutostartInterval == 0 {
minAutostartInterval = time.Duration(template.MinAutostartInterval)
}

if err := s.UpdateTemplateMetaByID(r.Context(), database.UpdateTemplateMetaByIDParams{
Copy link
Member

Choose a reason for hiding this comment

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

Question: Do we actually want to mutate the templates here, or should we update the template to a new revision? Motivation:

  • Mutation does not leave a trace of the change
  • It can become hard to explain how changes in template metadata affect (or don't affect) running workspaces

Copy link
Member Author

Choose a reason for hiding this comment

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

Mutation does not leave a trace of the change

Audit logging will cover this, I think

It can become hard to explain how changes in template metadata affect (or don't affect) running workspaces

I would argue strongly that template metadata changes should never affect running workspaces. However, if a template owner edits max_ttl or min_autostart_duration then edits to workspaces, running or not, will be affected.

Additionally, as things stand this would require moving the fields max_ttl and min_autostop_duration_ms to the template_versions table, with all of the associated plumbing.

Copy link
Member

Choose a reason for hiding this comment

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

Audit logging will cover this, I think

Ok, that's great! As long as there's a trace I think that's good enough.

I would argue strongly that template metadata changes should never affect running workspaces. However, if a template owner edits max_ttl or min_autostart_duration then edits to workspaces, running or not, will be affected.

This sounds like it would be fine, for now at least. This is probably the behavior the template editor is looking for. Would someone editing a workspace be able to observe the change when they make the edit? If not that part might be a bit unexpected but otherwise OK IMO.

Additionally, as things stand this would require moving the fields max_ttl and min_autostop_duration_ms to the template_versions table, with all of the associated plumbing.

That sounds like a bigger issue and not something we want to do in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would someone editing a workspace be able to observe the change when they make the edit?

Depends if they have the permission to view the template, I guess 🤔 which should be true as far as I know.

ID: template.ID,
UpdatedAt: database.Now(),
Description: desc,
MaxTtl: int64(maxTTL),
MinAutostartInterval: int64(minAutostartInterval),
}); err != nil {
return err
}

updated, err = s.GetTemplateByID(r.Context(), template.ID)
if err != nil {
return err
}
return nil
})
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: "Internal error updating template metadata.",
Detail: err.Error(),
})
return
}

if updated.UpdatedAt.IsZero() {
httpapi.Write(rw, http.StatusNotModified, nil)
return
}

httpapi.Write(rw, http.StatusOK, convertTemplate(updated, count))
}

func convertTemplates(templates []database.Template, workspaceCounts []database.GetWorkspaceOwnerCountsByTemplateIDsRow) []codersdk.Template {
apiTemplates := make([]codersdk.Template, 0, len(templates))
for _, template := range templates {
Expand Down
Loading