- Notifications
You must be signed in to change notification settings - Fork927
fix: do not skip properties on creating templates#5060
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
| assert.Equal(t, template.Description, updated.Description) | ||
| assert.Equal(t, template.DisplayName, updated.DisplayName) | ||
| // Icon is removed, as the API considers it as "delete" request | ||
| assert.Equal(t, "", updated.Icon) |
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.
This is really unfortunate. If somebody runscoder templates edit <template-name>, they will automatically remove the icon. This is an outcome of not having a clear difference betweendelete icon anddo not modify. Do you think that it's worth opening another issue?
EDIT:
Side issue - it isn't possible to clear the template description with UI. User can submit an empty description field, but the content is not deleted.
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.
Considering the behavior here:
Lines 481 to 496 in4b3d211
| // Update template metadata -- empty fields are not overwritten. | |
| name:=req.Name | |
| displayName:=req.DisplayName | |
| desc:=req.Description | |
| icon:=req.Icon | |
| maxTTL:=time.Duration(req.DefaultTTLMillis)*time.Millisecond | |
| ifname=="" { | |
| name=template.Name | |
| } | |
| ifdisplayName=="" { | |
| displayName=template.DisplayName | |
| } | |
| ifdesc=="" { | |
| desc=template.Description | |
| } |
I would definitely consider it a bug. It's inconsistent how fields are affected. A missing field (null) should produce no change, an empty string should remove it and a non empty string change it. Feel free to open an issue about this. 👍🏻
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.
| assert.Equal(t, template.Description, updated.Description) | ||
| assert.Equal(t, template.DisplayName, updated.DisplayName) | ||
| // Icon is removed, as the API considers it as "delete" request | ||
| assert.Equal(t, "", updated.Icon) |
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.
Considering the behavior here:
Lines 481 to 496 in4b3d211
| // Update template metadata -- empty fields are not overwritten. | |
| name:=req.Name | |
| displayName:=req.DisplayName | |
| desc:=req.Description | |
| icon:=req.Icon | |
| maxTTL:=time.Duration(req.DefaultTTLMillis)*time.Millisecond | |
| ifname=="" { | |
| name=template.Name | |
| } | |
| ifdisplayName=="" { | |
| displayName=template.DisplayName | |
| } | |
| ifdesc=="" { | |
| desc=template.Description | |
| } |
I would definitely consider it a bug. It's inconsistent how fields are affected. A missing field (null) should produce no change, an empty string should remove it and a non empty string change it. Feel free to open an issue about this. 👍🏻
While working on#3321 I found that
POST /templatesrejects properties, even though theAPI contract expects it. This PR fixes the behavior.