-
Notifications
You must be signed in to change notification settings - Fork 848
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
Datasource for VMware #953
Datasource for VMware #953
Conversation
54d61c0
to
9a7da28
Compare
It says the CLA validate failed. I did sign it (this morning). I suppose it just is not approved yet? |
Yeah the CLA check takes 1-2 days on our end, so probably early next week |
Thanks @powersj , I will fix the lint issues. I also thought I removed the dependency on |
669f514
to
4c7cd9c
Compare
Hi @powersj, I was looking to see if it possible to have a datasource respond to two different names, and while that does not appear to be the case, I did find that this datasource no longer fully matches the DS docs. I am going to update this DS with an RST doc instead of the MD one I included, as well as determine which of the current methods the DS is not implementing. While I know the DS works, I'd rather it be merged at a baseline that is congruent with the current criteria. Thanks! |
With regards to the CLA, I can see that you've signed the CLA, but you haven't yet added yourself to the CLA signers file. As part of this PR, please also add your name (alphabetically) to the CLA signers file. The full details are described in the last bullet point of the documentation. Based on your last comment, it sounds like there's still more work you want to do as part of this PR. Are you looking for review feedback now, or would you like to make the updates first? |
Thanks for the question! Well, you can provide feedback if you like :) I think all I'm going to do for now is:
Anything else can come later, or as part of the review. I realize I'm missing some functions in the DS prototype per the current documentation. I'd rather y'all let me know what is missing. Some things are a result of maintaining Py2 compat, but that's no longer a concern once merged into CI proper. |
4c7cd9c
to
1c9b6f5
Compare
Okay, I've updated the name and docs. Have at ye! :) |
5621f8a
to
ad42b84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good overall. I'm used to seeing cloud-init retrieving the network information from the datasource to apply to the machine and not the other way around, so this is a bit unique in that regard.
I left a number of inline comments, but most are pretty minor. The one major thing is the use of netifaces. We're a bit wary of adding new dependencies to cloud-init. We have some similar networking reporting tools in cloudinit/netinfo.py. Would you be able to use the functions there instead of pulling in netifaces
?
Additionally, there are a few places where you reference support for older versions of cloud-init. Since this datasource will ship as part of whatever version of cloud-init we're shipping, compatibility with older versions won't be a concern anymore. I highlighted these inline as well.
You may want to add VMware to Supported Private Clouds
section in README.md
.
Each datasource has its own set of unit tests under tests/unittests/test_datasource. Would you be able to add your own?
Hi @TheRealFalcon, Thank you for the review! Before I respond/address/fix anything else, I wanted to comment on the Looking at Netifaces supports Windows as well. While that's not relevant here per se, Cluster API (CAPI), the Kubernetes project for bootstrapping Kube clusters with Kubernetes, supports Linux and Windows nodes. The bootstrap controller produces cloud-init payloads for Linux guests, and in order to support Windows, the CAPI team worked with Microsoft to port this datasource to cloudbase-init in order to make it as seamless as possible for CAPI and its infrastructure providers (Azure, AWS, VMware, etc.) to support bootstrapping Windows nodes. That's a long way to say, can I pretty please continue to use |
Only because you said pretty please 😉 . I remember the conversations about netifaces and I also remember concluding that it shouldn't be too hard to replace it, so that's why I suggested it here. Your reasoning makes sense though, and I think it's ok to keep it currently, but I would also like to get a +1 from @blackboxsw on this. |
ad42b84
to
6e50d86
Compare
Hi @TheRealFalcon,
Absolutely, of course. The only reason I did not was because I'm not sure how to mock the RPC calls. I can create tests for everything post the actual retrieval of the data. The other challenge is the fact the datasource relies on inspecting the host's network configuration. I am not sure how I should mock that so the answer is deterministic enough to be testable. Do you have suggestions on how to do either? FWIW, this is why testing for me has always been firing up a VM on my desktop in Fusion and verifying the outcome. The datasource's RPC calls via I'm more than happy to add unit tests if we can figure out how to address the two items above. Heck, I'm happy to add some even without addressing those items, but given those items should be tested, I would sure appreciate helping figuring out how to do so. |
6e50d86
to
31cf845
Compare
In general there's some helper base classes that do a lot of the mocking for you. Just as an example, if you take a look at test_digitalocean.py, you can see it inherits from Does that help at all?
Yeah, unit testing is frustrating when we have to mock so much. It definitely helps in regression testing though. We do also have an integration testing framework that lets us do some more real-world test cases. These tests rely heavily on another library called pycloudlib. The scope would be beyond this PR, but if we could make VMware a supported "cloud" in pycloudlib, it would it very simple to write an integration test for this datasource that is end-to-end and doesn't require any mocking. I'll admit that I'm fairly oblivious to the VMware ecosystem, so I would need to come up to speed on how the VMs are launched and managed, but if you're interested in more representative tests, this would be the way to go. It'd be a welcome addition into pycloudlib. |
31cf845
to
bff7029
Compare
HI @TheRealFalcon, Holy. crap.
|
6b120ee
to
3e994d3
Compare
This patch adds a note to the README that indicates the repo is now deprecated since the datasource has been merged into cloud- init with canonical/cloud-init#953.
VMware support was added under #953.
VMware support was added under canonical#953. Also includes the python3-netifaces dependency in d/control
VMware support was added under canonical#953. Also includes the python3-netifaces dependency in d/control
VMware support was added under canonical#953. Also includes the python3-netifaces dependency in d/control
VMware support was added under canonical#953. Also includes the python3-netifaces dependency in d/control
VMware support was added under #953. Also includes the python3-netifaces dependency in d/control
VMware support was added under #953. Also includes the python3-netifaces dependency in d/control
VMware support was added under #953. Also includes the python3-netifaces dependency in d/control
VMware support was added under canonical#953.
Proposed Commit Message
Additional Context
Test Steps
Checklist: