Add GetVirtualHardwareSection fuction #200

frenchtoasters · 2019-06-12T03:14:40Z

IMPORTANT

To help us process your pull request efficiently, please follow the
guidelines shown below.

A Pull Request should be associated with an Issue

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.

We accept PRs without associated issues provided the change is sufficiently evident
from the commit message. If you have typos or simple bug fixes go for it.

Description

Related issue: #199

Add GetVirtualHardwareSection fuction

This function is near identical to GetNetworkConnectionSection() in /govcd/vm.go and does essentially the same thing. It gathers the /vApp/{id}/virtualHardwareSection endpoint of a VM from the restapi and then returns that the *types.VirtualHardwareSection pointer which already has the corresponding structs written, here. It also references a new mime type types.MimeVirtualHardwareSection which was derived from here.

Returning the Vdc.Status as a string breaks the usage of the `types.VDCStatuses[]` map as the value returned is of `type string` and not `type int` as the map is defined here https://github.com/vmware/go-vcloud-director/blob/master/types/v56/types.go#37. Currently something like this is required to use the `types.VDCStatues[]` map: ``` i, err := strconv.Atoi(vdc.Vdc.Status) if err != nil { return err } log.Info(types.VDCStatuses[i]) // Returns the mapped value ``` This change would give `Vdc.Status` the same behavior as `Vapp.Status` when using either of their corresponding maps and allow for this to work: ``` log.Info(types.VDCStatuses[vdc.Vdc.Status]) ```

Didainius · 2019-06-12T08:38:44Z

Hi, @frenchtoasters. Thanks for the PR.
Would you be able to pull in a test for the new function?

govcd/vm.go

vbauzys

Thank you for your PR - just small improvements needed. And also unit test will be big help.

types/v56/types.go

frenchtoasters · 2019-06-12T13:27:44Z

Hi, @frenchtoasters. Thanks for the PR.
Would you be able to pull in a test for the new function?

Ya sure I didnt see one for the GetNetworkConnectionSection but I can mock one up tonight for this.

govcd/vm_test.go

lvirbalas · 2019-06-18T12:03:27Z

@frenchtoasters as a preparation for merge, please provide a commit message following the guidelines https://chris.beams.io/posts/git-commit/ as referenced in CONTRIBUTING.md

dataclouder · 2019-06-18T12:37:12Z

govcd/vm_test.go

+		},
+	}
+
+	vm, err := vcd.client.Client.FindVMByHREF(newVM.HREF)


This call cannot work.
You are calling a function that finds a VM by HREF, but the HREF is empty, as it is only available for entities that have been created or fetched from vCD.

The test should use an already existing VM (preferred), or create a new one, and then get its contents.

lvirbalas · 2019-07-08T14:05:15Z

@frenchtoasters Are you planning to complete this one?

frenchtoasters · 2019-07-08T16:00:04Z

@frenchtoasters Are you planning to complete this one?

Yes I do just have not been able to budget out the time to correct the unit test. Though I believe the fix is fairly simple, in that I should just be using the findFirstVapp and findFirstVm functions instead of creating my own vm.

Though I am wondering @dataclouder would it be acceptable to set the VirtualHardwareSection for the VM that was found myself and then validate those values are returned correctly from the GetVirtualHardwareSection or should I use a SetVirtualHardwareSection function to create them as well?

dataclouder · 2019-07-08T16:11:03Z

Though I am wondering @dataclouder would it be acceptable to set the VirtualHardwareSection for the VM that was found myself and then validate those values are returned correctly from the GetVirtualHardwareSection or should I use a SetVirtualHardwareSection function to create them as well?

Both approaches work, though I am not sure the first one could get predictable results.
I would accept both, provided that:
a) the tests are done with real VMs, not mocks.
b) the results are independently verifiable (meaning that creating vApps/VMs with our own OVAs, we get the same results as you do)
c) the testing code has sufficient comments to explain what is the test intention and what's the expected result.

Looking forward to evaluating your improvements!

Didainius

Hi @frenchtoasters,
Sorry for leaving you in limbo for this long. We had the release on tight schedule.

I have ran your test and it failed on my side.

go test -tags functional -check.vv -check.f "Test_GetVirtualHardwareSection" -timeout=15m .

START: vm_test.go:815: TestVCD.Test_GetVirtualHardwareSection
Running: TestGetVirtualHardwareSection
vm_test.go:851:
    check.Assert(item.Address, Equals, vm.VM.VirtualHardwareSection.Item[0].Address)
... obtained string = "00:50:56:29:01:fd"
... expected string = "1.1.1.1"

FAIL: vm_test.go:815: TestVCD.Test_GetVirtualHardwareSection

I have left some ideas inline on how to make this working.

go.sum

govcd/vm_test.go

Co-Authored-By: Dainius <Didainius@users.noreply.github.com>

Didainius · 2019-08-26T11:42:39Z

The tests seems to be passing for me. Could you merge latest master branch as it has conflicts now.

Didainius · 2019-08-26T11:43:04Z

Be sure to make fmt and see that TravisCI is green.

govcd/vm.go

vbauzys

LGTM

Co-Authored-By: Dainius <Didainius@users.noreply.github.com>

Didainius · 2019-08-27T12:58:30Z

Thanks!
The test passes, looks fine. I'll let other team members look around and approve before we merge it.

dataclouder

LGTM

lvirbalas

Thanks for contributing!

Didainius · 2019-08-28T12:59:53Z

It is now merged. Thank you @frenchtoasters !

frenchtoasters and others added 5 commits June 10, 2019 23:45

running make fmt

51c0818

addition to add getvirtualhardwaresection function and mime

c310542

fixing typo, running formating

ac6fd3f

replacing local vapp_test.go with repos

0ee179e

Didainius requested review from lvirbalas, vbauzys and dataclouder and removed request for vbauzys June 12, 2019 08:37

vbauzys reviewed Jun 12, 2019

View reviewed changes

govcd/vm.go Outdated Show resolved Hide resolved

vbauzys reviewed Jun 12, 2019

View reviewed changes

govcd/vm.go Show resolved Hide resolved

vbauzys suggested changes Jun 12, 2019

View reviewed changes

types/v56/types.go Outdated Show resolved Hide resolved

toast added 4 commits June 12, 2019 19:31

adding tests and comments

c8eb1b6

move test around and update

4e4ea75

reverting changed file

1862198

reverting changed file

21beeb3

vbauzys reviewed Jun 13, 2019

View reviewed changes

govcd/vm_test.go Outdated Show resolved Hide resolved

adding data to test

332f341

dataclouder reviewed Jun 18, 2019

View reviewed changes

toast added 3 commits July 9, 2019 18:44

update test to find first vapp/vm

0c53f4a

adding back removed dep from vm_test

6a6ba53

Adding comments

f45ef0d

Didainius reviewed Jul 31, 2019

View reviewed changes

go.sum Outdated Show resolved Hide resolved

govcd/vm_test.go Outdated Show resolved Hide resolved

govcd/vm_test.go Show resolved Hide resolved

govcd/vm_test.go Show resolved Hide resolved

updating vm_test and tiding up go.sum

b16647a

Didainius reviewed Aug 21, 2019

View reviewed changes

govcd/vm_test.go Outdated Show resolved Hide resolved

frenchtoasters and others added 2 commits August 24, 2019 11:51

Update govcd/vm_test.go

5ca8c1e

Co-Authored-By: Dainius <Didainius@users.noreply.github.com>

removing condition on if statment.

6d558bf

toast and others added 3 commits August 26, 2019 17:03

merging latest master

3cd073b

fixing conflict in vm_test.go

5690231

Merge branch 'master' into patch-2

847d5ca

Didainius reviewed Aug 27, 2019

View reviewed changes

govcd/vm.go Outdated Show resolved Hide resolved

vbauzys approved these changes Aug 27, 2019

View reviewed changes

Update govcd/vm.go

4479674

Co-Authored-By: Dainius <Didainius@users.noreply.github.com>

dataclouder approved these changes Aug 28, 2019

View reviewed changes

lvirbalas approved these changes Aug 28, 2019

View reviewed changes

Didainius merged commit 7f5c3df into vmware:master Aug 28, 2019

Didainius added a commit to Didainius/go-vcloud-director that referenced this pull request Aug 29, 2019

Add note to changelog about PR vmware#200

fb4c821

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetVirtualHardwareSection fuction #200

Add GetVirtualHardwareSection fuction #200

frenchtoasters commented Jun 12, 2019

Didainius commented Jun 12, 2019

vbauzys left a comment

frenchtoasters commented Jun 12, 2019

lvirbalas commented Jun 18, 2019 •

edited

Loading

dataclouder Jun 18, 2019 •

edited

Loading

lvirbalas commented Jul 8, 2019

frenchtoasters commented Jul 8, 2019

dataclouder commented Jul 8, 2019

Didainius left a comment

Didainius commented Aug 26, 2019

Didainius commented Aug 26, 2019

vbauzys left a comment

Didainius commented Aug 27, 2019

dataclouder left a comment

lvirbalas left a comment

Didainius commented Aug 28, 2019

Add GetVirtualHardwareSection fuction #200

Add GetVirtualHardwareSection fuction #200

Conversation

frenchtoasters commented Jun 12, 2019

IMPORTANT

A Pull Request should be associated with an Issue

Description

Add GetVirtualHardwareSection fuction

Didainius commented Jun 12, 2019

vbauzys left a comment

Choose a reason for hiding this comment

frenchtoasters commented Jun 12, 2019

lvirbalas commented Jun 18, 2019 • edited Loading

dataclouder Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

lvirbalas commented Jul 8, 2019

frenchtoasters commented Jul 8, 2019

dataclouder commented Jul 8, 2019

Didainius left a comment

Choose a reason for hiding this comment

Didainius commented Aug 26, 2019

Didainius commented Aug 26, 2019

vbauzys left a comment

Choose a reason for hiding this comment

Didainius commented Aug 27, 2019

dataclouder left a comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Didainius commented Aug 28, 2019

lvirbalas commented Jun 18, 2019 •

edited

Loading

dataclouder Jun 18, 2019 •

edited

Loading