feat: Update workspace for templates server side #9272
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9272 +/- ##
=======================================
Coverage 44.60% 44.60%
=======================================
Files 1273 1273
Lines 155831 155852 +21
Branches 2439 2439
=======================================
+ Hits 69504 69520 +16
- Misses 86088 86093 +5
Partials 239 239
Flags with carried forward coverage won't be shown. Click here to find out more.
|
master/internal/templates/api.go
Outdated
| case errors.Is(err, db.ErrNotFound): | ||
| _, err := a.PostTemplate(ctx, &apiv1.PostTemplateRequest{Template: req.Template}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &apiv1.PutTemplateResponse{Template: req.Template}, nil |
There was a problem hiding this comment.
Why is this a special case?
master/internal/templates/api.go
Outdated
| permErr, err := AuthZProvider.Get().CanUpdateTemplate( | ||
| ctx, user, model.AccessScopeID(tpl.WorkspaceID), | ||
| ) | ||
| switch { |
There was a problem hiding this comment.
Why use a switch instead of if statements here? (I'm wondering if it's a style convention I'm not familiar with?)
There was a problem hiding this comment.
This part was moved from other places, I saw we use both switch and if under similar circumstances.
master/internal/templates/api.go
Outdated
| req := &apiv1.PatchTemplateConfigRequest{ | ||
| TemplateName: req.Template.Name, | ||
| Config: req.Template.Config, | ||
| user, _, err := grpcutil.GetUser(ctx) |
There was a problem hiding this comment.
nit: Since there's more complexity in the default case, and nothing else happens in the function, I would prefer simplifying the default case a bit. Either leave off default: and let everything that happens in default: happen as part of normal function execution. Or, if it makes sense as a logical unit, move some of it into separate functions.
|
|
||
| resp1, err := api.PutTemplate(ctx, &apiv1.PutTemplateRequest{Template: input}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, resp1.Template.WorkspaceId, int32(w.ID)) |
There was a problem hiding this comment.
Can we add a test for an invalid workspace ID too if it doesn't already exist?
| exists, err := workspace.Exists(ctx, int(workspaceID)) | ||
| switch { | ||
| case err != nil: | ||
| return fmt.Errorf("failed to check workspace %d: %w", workspaceID, err) |
There was a problem hiding this comment.
How about workspace check failed instead of failed to check workspace? I think it's a little simpler and more descriptive. When I first read failed to check workspace I thought it was saying the user had failed to do something and then I read it again.
There was a problem hiding this comment.
I see now that this function is not new as much as moved ... feel free to ignore if you'd prefer.
| ) | ||
| switch { | ||
| case err != nil: | ||
| return fmt.Errorf("failed to check for permissions: %w", err) |
There was a problem hiding this comment.
Similar to above, how about permission check failed?
master/internal/templates/api.go
Outdated
| ) | ||
| switch { | ||
| case err != nil: | ||
| return nil, fmt.Errorf("failed to check for permissions: %w", err) |
There was a problem hiding this comment.
permission check failed? (similar comments below with more explanation)
Ticket
MD-379
Description
In order to support move templates between workspaces, update
PutTemplateto take workspace id.Test Plan
Change workspace id for a template through
/api/v1/templates/{templateName}endpoint.Using invalid workspace id should not go through.
Checklist
docs/release-notes/.See Release Note for details.