Added virtual machine filtering #255
Conversation
|
Hey @abhi1693 thanks for the PR! Before merging we do require that at least unit tests, but also typically integration tests are written and passing. If you need help in writing tests - attend one of the Test Clinics on Twitch. |
|
Looks like pre-commit is also failing |
I checked and there are no unit tests for |
abhi1693
force-pushed
the
feat/vm-filter
branch
from
087d837
to
fab4190
Compare
|
@waynew Can you please review the code now? |
|
@abhi1693, VMware has approved your signed contributor license agreement. |
|
@abhi1693 sorry for the late response - life has been busy on this side! I'm also looking at #280 right now, and I'm seeing kind of the issue when it comes to actually running these integration tests. Basically, we made some design choices when we started out that we need to change courses on now. In particular, we didn't even have the ability or experience with vSphere to actually spin up test instances and really know what we were doing 😅 So we just kind of... guessed. Which worked OK for most things, but here it's pretty clear that we've reached the point where that approach no longer serves us well. The overall plan is that we need to actually connect to our vSphere and spin up the expected test topolog(y|ies) that we need, and test against those. I'm working right now to get that setup for #280. But that's the integration tests. We could still have some unit tests that just kind of mock the pyvmomi layer (or potentially the utils layer, if the change is in the state/module layer) which would allow running tests without requiring vSphere access. |
|
@waynew I'm not sure if you are asking me to do something here. Regarding the unit tests, I'll need some samples that can help me write something for this PR. |
|
@abhi1693 I'm going to take a look a this on today's Test Clinic - you can either attend live or catch up after at https://twitch.tv/saltprojectoss |
|
@abhi1693 apologies for the abrupt end to the stream - I actually lost power for 2 hours last night when everything went sideways 😂 |
|
@abhi1693 https://github.com/saltstack/salt-ext-modules-vmware/compare/main...waynew:salt-ext-modules-vmware:tc/start-vm-tests?expand=1 just pushed the code that I had at the end of test clinic. If you still think you might need some more info, I'd be happy to help out on tomorrow's Test Clinic as well! |
|
Once I start maybe I'll be able to get something done. I'll try to join today as well. |
|
@abhi1693 was planning to continue today, but I'm out sick so I won't be streaming today 🤒 If I'm feeling better Thursday, can plan on then but in all likelihood it probably won't be until next Tuesday or Thursday |
|
@abhi1693 looks like there are some conflicts and (maybe?) test failures - do you need any help looking into that? |
|
@waynew I'll need some help on this because none of the tests is failing on my local system either. I'm not so sure why CI is throwing that error and how to reproduce it. I have tested this on Python 3.7.14 as well with no errors. |
|
Ah! I was able to repro - gotta merge the branch into main 👍 |
|
looks like that only fails with 3.7 on main, tho... |
|
okay! I got it, something weird with mock and calls. I opened a PR against your fork, get that merged and then things should be passing here and then I can merge if there are no objections! 👍 |
Feat/vm filter
Closes: #254