Add functions to allow to update Vdc storage profile #340
Conversation
Merge branch 'master' of github.com:vmware/go-vcloud-director
Merge branch 'master' of github.com:vmware/go-vcloud-director
Merge branch 'master' of github.com:vmware/go-vcloud-director
# Conflicts: # govcd/api.go # govcd/api_vcd_test.go # govcd/openapi_endpoints.go # types/v56/constants.go # types/v56/openapi.go
vbauzys
requested review from
lvirbalas,
dataclouder and
Didainius
and removed request for
lvirbalas and
dataclouder
types/v56/types.go
Outdated
| NetworkQuota int `xml:"NetworkQuota,omitempty"` | ||
| VmQuota int `xml:"VmQuota,omitempty"` | ||
| IsEnabled bool `xml:"IsEnabled,omitempty"` | ||
| VdcStorageProfile []*CreateVdcStorageProfile `xml:"VdcStorageProfile"` |
There was a problem hiding this comment.
This is getting confusing. Please add a comment why "CreateVdcStorageProfile" in "VdcStorageProfile".
There was a problem hiding this comment.
it was hard to grasp for me too, until I figured out that API uses 3 types or VDC storage profile types which are almost incidental. Some of them need different XML root name. Every type has links to API description type. In general CreateType is used when creating Org VDC - which has less fields, VdcStorageProfile is used for GET and AdminVdcStorageProfile for update.
There was a problem hiding this comment.
Please add the explanation as comment in code.
There was a problem hiding this comment.
Where too? all this type are referenced in different locations and every type already has explanation for what it used and reference to API.
There was a problem hiding this comment.
Could we possible call it AttachVdcStorageProfile or something similar. The CreateVdcStorageProfile sounds as if it is going to create a storage profile but in fact it is not (storage profiles are defined in pVdcs)
There was a problem hiding this comment.
Well result will be I have to introduce new functions for createOrgVdc, createOrgVdcType then +4 VdcStorageProfile types and new functions for getStorageProfilHref and everything will be with "silly namings" and deprecate 2 functions. Overall this minimum I recalled in my head. @lvirbalas what to do?
There was a problem hiding this comment.
After discussion we renamed to VdcStorageConfiguration to be inline with VdcConfiguration - structs that are used to create the VDC.
@dataclouder do you have an alternative proposal without deprecations? One alternative I can think of is actually leaving old name as is, but adding a suffix or prefix "Read" to the new structs:
VdcStorageProfileRead and AdminVdcStorageProfileRead.
Either way, we need to be consistent with these kind of prefixes going forward.
There was a problem hiding this comment.
if we do that, we break get function definitions. So more breakage
There was a problem hiding this comment.
leaving old name as is, but adding a suffix or prefix "Read" to the new structs:
VdcStorageProfileRead and AdminVdcStorageProfileRead.
That I could live with. And it would make it possible to deprecate the old one.
Unless I misunderstood the suggestion. "As is" means as it is now or as it was before the PR?
There was a problem hiding this comment.
Let's go with VdcStorageProfileConfiguration, just last ask to add breaking changes section to changelog.
types/v56/types.go
Outdated
| NetworkQuota int `xml:"NetworkQuota,omitempty"` | ||
| VmQuota int `xml:"VmQuota,omitempty"` | ||
| IsEnabled bool `xml:"IsEnabled,omitempty"` | ||
| VdcStorageProfile []*CreateVdcStorageProfile `xml:"VdcStorageProfile"` |
There was a problem hiding this comment.
Could we possible call it AttachVdcStorageProfile or something similar. The CreateVdcStorageProfile sounds as if it is going to create a storage profile but in fact it is not (storage profiles are defined in pVdcs)
# Conflicts: # CHANGELOG.md
added |
# Conflicts: # CHANGELOG.md
This new function will allow to update VDC storage profile.