Skip to content
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

Add support for handling catalog storage profiles #345

Merged
merged 5 commits into from
Dec 9, 2020

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Dec 1, 2020

It can be tricky to use catalogs while working with multiple VDCs without specifying particular storage profile (vmware/terraform-provider-vcd#598)

This PR adds functions to SDK so that one can handle Storage profiles for catalog during creation and update phases.

It improves functions func (adminCatalog *AdminCatalog) Update() and adds

  • func (adminOrg *AdminOrg) CreateCatalogWithStorageProfile
  • func (org *Org) CreateCatalogWithStorageProfile

Additionally these functions are added for convenience:

  • func (adminOrg *AdminOrg) GetAllStorageProfileReferences
  • func (adminOrg *AdminOrg) GetStorageProfileReferenceById
  • func (adminOrg *AdminOrg) GetAllVDCs

Note. Test suite passed on at least one env.

@Didainius Didainius self-assigned this Dec 1, 2020
@Didainius Didainius marked this pull request as ready for review December 2, 2020 06:32
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements for handling catalog

@@ -42,6 +42,74 @@ func (adminOrg *AdminOrg) CreateCatalog(name, description string) (AdminCatalog,
return CreateCatalog(adminOrg.client, adminOrg.AdminOrg.Link, name, description)
}

// CreateCatalogWithStorageProfile is like CreateCatalog, but allows to specify storage profile
func (adminOrg *AdminOrg) CreateCatalogWithStorageProfile(name, description string, storageProfiles *types.CatalogStorageProfiles) (AdminCatalog, error) {
return CreateCatalogWithStorageProfile(adminOrg.client, adminOrg.AdminOrg.Link, name, description, storageProfiles)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that catalog can have assigned multi storage profiles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, but API structure is such. The other way is I could always wrap it in this function. I just accepted this structure as it looked more convenient (and maybe future proof although I know it may never happen).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well due this naming and type I got strange impression that multi storage profiles supported:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the structure accepts a slice, but I left it as I see it as a possibility for future. If it can use "any of all", why can't it use "any of specified".

return CreateCatalogWithStorageProfile(adminOrg.client, adminOrg.AdminOrg.Link, name, description, storageProfiles)
}

// GetAllVDCs returns all child VDCs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "all depending VDCs" because using "child" as adjective for plural recipients doesn't sound right

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return allVdcs, nil
}

// GetAllStorageProfileReferences traverses all child VDCs and returns a slice of storage profile references
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"all depending VDCs"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return allStorageProfileReferences, nil
}

func (adminOrg *AdminOrg) GetStorageProfileReferenceById(id string, refresh bool) (*types.Reference, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@@ -91,13 +91,19 @@ func (org *Org) GetVdcByName(vdcname string) (Vdc, error) {
}

func CreateCatalog(client *Client, links types.LinkList, Name, Description string) (AdminCatalog, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing func description

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@@ -603,7 +603,7 @@ type ComputeCapacity struct {
// Description: A reference to a resource. Contains an href attribute and optional name and type attributes.
// Since: 0.9
type Reference struct {
HREF string `xml:"href,attr"`
HREF string `xml:"href,attr,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case does a Reference return an empty HREF?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact this was not for return, but for "POST"/"PUT". When attaching storage profile to catalog I wanted to use IDs (because they are what we use and having only ID is sufficient for the task). What happened is it would send ID and empty HREF field (href="")which caused validation error.

CHANGELOG.md Outdated
@@ -8,6 +8,9 @@
* Added functions `edge.GetLbAppRules`, `edge.GetLbServerPools`, `edge.GetLbAppProfiles`, `edge.GetNsxvNatRules`, `client.GetOrgList`
* Exported private function `client.maxSupportedVersion` to `client.MaxSupportedVersion`
* Able to upload an OVF without ovf:size defined in File part. Some bug fix for uploading OVA/OVF. [#331](https://github.com/vmware/go-vcloud-director/pull/331)
* Add support for handling catalog storage profiles (`adminOrg.CreateCatalogWithStorageProfile`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that catalog can only have ONE storage profile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this as a general term "storage profiles" and yes - it is true that it only handles one. Will change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks neat!

@Didainius Didainius merged commit c70f758 into vmware:master Dec 9, 2020
@Didainius Didainius deleted the catalog_storage_profiles2 branch December 9, 2020 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants