-
Notifications
You must be signed in to change notification settings - Fork 51
feat: storage box api changes #745
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## storage-boxes #745 +/- ##
==============================================
Coverage 78.38% 78.39%
==============================================
Files 59 59
Lines 5561 5600 +39
==============================================
+ Hits 4359 4390 +31
- Misses 912 917 +5
- Partials 290 293 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I did a quick pass and everything looks good to me so far |
// StorageBoxUpdateRequest defines the schema of the request to update a Storage Box. | ||
type StorageBoxUpdateRequest struct { | ||
Name *string `json:"name,omitempty"` | ||
Name string `json:"name,omitempty"` |
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 change feels backward. The fields in update requests will become optional for all three resources.
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.
The pointer was orignally there, because the API allowed empty strings to be set as a Storage Box name. This won't be the case in the future and an empty name will be ignored.
I thought this makes it somewhat easier to the user, as a pointer is not required.
We have a mixed pattern in the codebase, so I was unsure what the best option is. Both are fine for me.
// StorageBoxUpdateRequest defines the schema of the request to update a Storage Box.
type StorageBoxUpdateRequest struct {
Name string `json:"name,omitempty"`
// ...
}
// NetworkUpdateRequest defines the schema of the request to update a network.
type NetworkUpdateRequest struct {
Name string `json:"name,omitempty"`
// ...
}
type LoadBalancerUpdateRequest struct {
Name *string `json:"name,omitempty"`
// ...
}
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.
I prefer without pointer, reduces the complexity.
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.
@apricote I would go ahead and merge this now, to proceed with the other review points in the main PR. We can discuss this topic there again, if you want.
- remove redundant parsing of full storage box properties - make tests more coherent between storage box and the subressources snapshot and subaccount
expected := `{ | ||
"home_directory": "/foobar" | ||
}` |
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.
🤩
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.
The changes look good, I have comments on the code that didn't change though.
No description provided.