- Notifications
You must be signed in to change notification settings - Fork928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
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 abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
| 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) { | ||
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. Hmm, should we add some linting for this? The actual (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 commentThe 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 commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, there was a case for using 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. Yeah, might be a good topic for dicussion! Member 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. @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 { | ||
johnstcn marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| minAutostartInterval = time.Duration(template.MinAutostartInterval) | ||
| } | ||
johnstcn marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| 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 shouldnever 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 { | ||
Uh oh!
There was an error while loading.Please reload this page.