-
Notifications
You must be signed in to change notification settings - Fork 887
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
} |
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) | ||
}) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
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 { | ||
johnstcn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
minAutostartInterval = time.Duration(template.MinAutostartInterval) | ||
} | ||
johnstcn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if err := s.UpdateTemplateMetaByID(r.Context(), database.UpdateTemplateMetaByIDParams{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Audit logging will cover this, I think
I would argue strongly that template metadata changes should never affect running workspaces. However, if a template owner edits Additionally, as things stand this would require moving the fields There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, that's great! As long as there's a trace I think that's good enough.
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.
That sounds like a bigger issue and not something we want to do in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 { | ||
|
There was a problem hiding this comment.
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:(Note: I'm not suggesting we fix the usage in this PR).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andNew
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 noticedErrorf
was also marked deprecated in favor offmt.Errorf
even though the latter does not include a stack trace. Weird.There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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