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

Set the InventoryPath of the folder object in DefaultFolder #1515

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

ykakarap
Copy link
Contributor

@ykakarap ykakarap commented Jul 3, 2019

The DefaultFolder is called in FolderOrDefault function which is expected to return a folder object of the default folder if the given path the empty.

The DefaultFolder object after creating the folder object needs to set the InventoryPath.

This PR fixes the above-mentioned issue.

find/finder.go Outdated
@@ -916,6 +916,13 @@ func (f *Finder) DefaultFolder(ctx context.Context) (*object.Folder, error) {
}
folder := object.NewFolder(f.client, ref.Reference())

// Set the InventoryPath of the newly created folder object
e, err := f.Element(ctx, folder.Reference())
Copy link
Member

Choose a reason for hiding this comment

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

In this case, we can avoid calling f.Element, since we have the Datacenter already and the vmFolder name is always "vm". I think the following should be ok:

folder.SetInventoryPath(path.Join(f.dc.Datacenter.InventoryPath, "vm"))

Can you try that out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this approach and it works. Pushed a new commit with the change.

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Great, thanks @ykakarap , I'll merge once CI completes.

@dougm dougm merged commit 35d0b7d into vmware:master Jul 3, 2019
@dougm
Copy link
Member

dougm commented Jul 3, 2019

Thanks @ykakarap !

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.

3 participants