Dev/cloud vultr

[richardsimko]

What does this PR do?

Improves the Vultr provider for Salt Cloud by adding options for firewallgroupid (#53677) and SSH keys.

What issues does this PR fix or reference?

Fixes: #53677

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

[Ch3LL]

Just a question and one change request. We will also need some test coverage for this change.

[richardsimko]

Thanks for the feedback. As there are no test cases at all for the Vultr client and I don't feel comfortable taking full ownership over writing those it would be very hard to write tests exclusively covering this small change, which is why I left it out.

[Ch3LL]

Here's a test to help get you started:

(salt-3.7) ➜  salt git:(dev/cloud-vultr) ✗ cat tests/unit/cloud/clouds/test_vultr.py
# -*- coding: utf-8 -*-

# Import Salt Libs
from salt.cloud.clouds import vultrpy as vultr

# Import Salt Testing Libs
from tests.support.unit import TestCase
from tests.support.helpers import TstSuiteLoggingHandler


class VultrTestCase(TestCase):
    """
    Unit TestCase for salt.cloud.clouds.vultr module.
    """
    def test_show_keypair_no_keyname(self):
        """
        test salt.cloud.clouds.vultr.show_keypair
        when keyname is not in kwargs
        """
        kwargs = {}
        with TstSuiteLoggingHandler() as handler:
            assert not vultr.show_keypair(kwargs)
            assert "ERROR:A keyname is required." in handler.messages

Let me know if you need more direction to add test coverage for the rest of the code. Thanks :)

[Ch3LL]

Also to be clear you only need to add coverage for your change, not for the entire module.

[richardsimko]

Thanks for the guidance. I added that and another test, however most of the added logic is in the create function which is not very testable, I don't see any of the other test providers having test coverage for this function either, probably due to the heavy coupling to other parts and the amount of mocking which would be required. Unless we completely rewrite it that will remain quite a challenge to be tested.

[Ch3LL]

I can definitely help in writing a test for create. Can you just paste here what kind of information you would expect to find in ssh_key_names and firewall_group_id

[richardsimko]

Thanks! Sure thing, ssh_key_names is a comma separated list like dev1,dev2,salt-master each representing an SSH public key which has been uploaded to Vultr and is installed on the VM when created. firewall_group_id is a string representing an ID of a firewall configuration in Vultr.

[Ch3LL]

Thanks, I've added this to my backlog. should be able to get to it by next week :)

[Ch3LL]

I will need to bump this to next week, apologies.

[richardsimko]

No worries! We're not really using Vultr any more so this is more in the interest of getting it merged in case someone else uses it (Which I somewhat doubt since it's nigh on unusable without using SSH keys 😅 )

[Ch3LL]

@richardsimko i went ahead and added a couple tests. Can you review and make sure the returns and args are all correct?

[richardsimko]

@Ch3LL Sorry for the late reply, everything looks good to me! 🎉

[sagetherage]

Since this is feature can you also add a Release Notes, please?