- 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.
Conversation
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.
With theomitempty fixes, this LGTM! Had some small suggestions and questions but nothing major.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| minAutostartInterval = time.Duration(template.MinAutostartInterval) | ||
| } | ||
| if err := s.UpdateTemplateMetaByID(r.Context(), database.UpdateTemplateMetaByIDParams{ |
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.
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
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.
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 shouldnever affect running workspaces. However, if a template owner editsmax_ttl ormin_autostart_duration thenedits to workspaces, running or not, will be affected.
Additionally, as things stand this would require moving the fieldsmax_ttl andmin_autostop_duration_ms to thetemplate_versions table, with all of the associated plumbing.
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.
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 shouldnever affect running workspaces. However, if a template owner edits
max_ttlormin_autostart_durationthenedits 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_ttlandmin_autostop_duration_msto thetemplate_versionstable, with all of the associated plumbing.
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 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
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.
Do we not have acoder templates show?
We do not, apparently |
| 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}) | ||
| iferrors.Is(err, sql.ErrNoRows) { | ||
| ifxerrors.Is(err, sql.ErrNoRows) { |
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 actualxerrors.Is function is marked deprecated, and we have a lot of mixed usage in the code-base:
❯ rg xerrors.Is | wc -l20❯ rg '[^x]errors.Is' | wc -l104(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 usingErrorf 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
m.Match("errors.$f($*_)").Report("Use xerrors.$f to provide additional stacktrace information!")
This PR adds a CLI command template edit which allows updating the following metadata fields of a template:- Description- Max TTL- Min Autostart Interval
This PR adds a CLI command
template editwhich allows updating the following metadata fields of a template: