Further simplify enumeration of attached disks for VMware VMX builder

[DanHam]

This PR:

• Collates separate regexp's into one for greater efficiency
• Inlines the internals of the getAttachedDisk function as we no longer need to call for separate adapter types
• Removes the now defunct struct and variables

This should probably have gone in with #6229 so apologies for the repeat. However, I didn't really see it then for what ever reason. I've kept the rather lengthy comment in there to explain the regexp/background...

As usual, feedback and suggestions for improvement are welcome

[SwampDragons]

I like the simplification of the regex, so thanks for this. What do you think about keeping getAttachedDisks as a helper function and actually writing some tests to validate the regex/document what it's looking for?

[DanHam]

I kind of prefer the function 'inline' as opposed to keeping it as a helper function. I guess it just looks/feels cleaner to me - we're not calling that piece of code multiple times any more so I felt the function wasn't needed any more.

I do see your point, that having a separate, testable function has some value. That said, we're not too bad with our test coverage for step_clone_vmx.go - 78.4%. The coverage report clearly highlights some of the bits we've not covered (grab the raw html report HERE). Clearly we should be testing those bits, so if you're happy for me to proceed on this basis, I'll have a go a refactoring the tests to get those highlighted elements covered off.

I'm wondering if I should try and move my monster comment/explanation of the 'why' behind the regex into the test file instead. What do you think?

[SwampDragons]

I can respect that; normally I don't like making tiny functions for things that are only done once either. That said, I'd like tests for this regex more than I'd like the elegance of having it inline. Looking over step_clone_vmx_test.go I'm not seeing anything that tests how we create the diskFileNames slice. The more obscure the code (and I think most of us find RegExes fairly hard to read) the more I like some kind of sanity test around them. We should have probably added this test a while ago; but now it feels kind of necessary. If you can find a way to test that the regex parses out what we expect without pulling it into a separate function, I'd be open to that too.

[DanHam]

I'm not seeing anything that tests how we create the diskFileNames slice.

So the diskFilenames slice created using that regex gets written into the diskFullPaths slice HERE

We then store diskFullPaths in the statebag as disk_full_paths HERE

We then test disk_full_paths is set and meets with expectations HERE

So the 'correctness' of the diskFileNames slice, and the regex used to set its contents, is tested via the checks we make on the value of disk_full_paths...

[SwampDragons]

facepalm yep I was reading this backwards. I saw the test code generating the diskFullPaths variable and didn't realize it was just being used as the "expected", not as an input to the test. This is fine. Thanks for bearing with me.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.