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 moref id to VM record type #491

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Conversation

yusufozturk
Copy link
Contributor

This PR adds VM moref id to VM record type struct.

Moref is already included in VM requests but it wasn't in the struct, so XML deserialization wasn't filling that.

Moref id helps us to match vCloud VM with vCenter VM.

Here is the issue:
#490

Thanks.

@vmwclabot
Copy link
Member

@yusufozturk, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@adambarreiro
Copy link
Collaborator

Hi @yusufozturk,
Thanks a lot for the contribution.

Just a small suggestion, would it be possible to add a small assertion in Test_QueryVM in vdc_test.go?

vm, err := vcd.vdc.QueryVM(vcd.vapp.VApp.Name, vmName)
check.Assert(err, IsNil)
check.Assert(vm.VM.Name, Equals, vmName)

To check that vm.VM.Moref is populated and starts with vm- (if that's always the case, I'm not a big expert).

Currently that test is just checking the VM name so I thought this could help to improve it + validate your change.

Best regards.

@vmwclabot
Copy link
Member

@yusufozturk, VMware has approved your signed contributor license agreement.

@yusufozturk
Copy link
Contributor Author

@adambarreiro thanks. I added tests but somehow there are additional whitespaces. I will revert them back.

@yusufozturk
Copy link
Contributor Author

@adambarreiro ready. I don't know how to run tests yet but I made the changes according to other tests.

Please let me know if you need anything else.

Thanks.

@Didainius
Copy link
Collaborator

Hello @yusufozturk,
Thank you for this PR. Tests works and we're happy to have it. I have just 2 asks:

We're happy to merge it afterwards

@yusufozturk
Copy link
Contributor Author

@Didainius thanks. I've made the changes.

@Didainius Didainius removed the request for review from lvirbalas August 5, 2022 05:25
Copy link
Collaborator

@adambarreiro adambarreiro left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for the contribution!

@Didainius Didainius merged commit 3b99bc2 into vmware:main Aug 5, 2022
@Didainius
Copy link
Collaborator

@yusufozturk ,
Thanks for PR - I have merged it in and it is now in main. I have tagged the code with your patch v2.17.0-alpha.1 if you want to consume it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants