check if volume backing store format is present before creating #569

[cyril-s]

• cover case when backing volume xml doesn't contain format element
• add test for NewDefBackingStoreFromLibvirt
• interface StorageVol introduced to allow mocking
• libvirt.StorageVol is mocked using github.com/golang/mock
• NewDefBackingStoreFromLibvirt now accepts StorageVol interface on input

I used github.com/golang/mock for mocking since it seemed to me as a better option rather than writing mocking implementations of interfaces like it's done in libvirt/libvirt_domain_mock_test.go

Closes #569

[MalloZup]

thx lot for PR ! I will have look once I have free time. Looks promising thx!! 😍

[MalloZup]

@cyril-s so looked at this pr.

I think we can do a testacc testcase without introducing mocking and the golang interface which add complexity.

Here some pointers for you:
https://github.com/dmacvicar/terraform-provider-libvirt/blob/master/CONTRIBUTING.md#writing-acceptance-tests

and

terraform-provider-libvirt/libvirt/resource_libvirt_volume_test.go

Line 131 in 753afd8

func TestAccLibvirtVolume_BackingStoreTestByID(t *testing.T) {

let me know if you have any other question, and thx lot for fixing the issue. great job sofar

[cyril-s]

@MalloZup so I was thinking how to implement acceptance tests for this and realized that currently there is no way to create lvm pool using this plugin. Which is clearly stated in the doc actually (and to which I didn't pay enough attention apparently). It's possible to implement testcase by creating pool without help of this plugin but it seems to me that PR that kind of fixes unsupported functionality is not very relevant. So it seems to me that this fix should go in PR which implements lvm pool support (that is something I'd like to help with). What do you think?

[MalloZup]

Hi.! Sorry for delay. So I personally think we don't need to test. If we add 10 lines tests for 1 line I think we can accept your pr without mocking. I don't like mocking in general and a'so introducing interfaces or coding for mocking isn't good'. So feel free to remove the mock part and preserve the code without interfaces. Thx

[MalloZup]

I'm saying this in context of the llvm pool, because if it is difficult to create all this isn't worth to have complex automation

[cyril-s]

Ok. Removed everything except fix itself

[MalloZup]

@cyril-s great job, just in case. I assume you tested it manually?

After that lgtm

[MalloZup]

just waiting for manual tests confirmation before merging

[cyril-s]

Yes, and I've been using the plugin with this fix for a week. No problems so far

[MalloZup]

@cyril-s thx for pr merging