Add ability to set GCE node labels via Salt Cloud

[sentient-glare]

What does this PR do?

This PR adds the ability to set node labels via Salt Cloud in Google Compute Engine.

What issues does this PR fix or reference?

Fixes: #61245

New Behavior

See relevant issue for details.

Merge requirements satisfied?

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

• [x] Docs
• [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• [x] Tests written/updated

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[welcome[bot]]

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

• Community Wiki
• Salt’s Contributor Guide
• Join our Community Slack
• IRC on LiberaChat
• Salt Project YouTube channel
• Salt Project Twitch channel

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

[sentient-glare]

First time contributor - I am a little lost when it comes to updating documentation and tests. Happy to update the MR if I can get a little more guidance.

Tests - I see there is a tests/integration/cloud/clouds/test_gce.py but as far as I can tell this just sees whether an instance can be created and destroyed. As far as I can tell there are currently no GCE unit tests in tests/unit/. Should I be writing new tests or just ensuring that the existing integration tests run, with labels set on the resulting instance?

Documentation - let me know if there's anything else I should be updating.

[Ch3LL]

@waynew any ideas on how we could add better test coverage for this one?

[waynew]

tl;dr - I think the code is probably correct but the unit tests for this module (not just this PR) are pretty broken.

---

@Ch3LL / @sentient-glare - I went ahead and made some tweaks here on today's Test Clinic - you should be able to catch the replay at https://twitch.tv/saltprojectoss if you take a look in the next few days. I also pushed my changes to https://github.com/waynew/salt/pull/new/tc/gce-test-tweaks

I have some pretty significant concerns -- not so much about this PR which looks good AFAICT -- but more like the test cases and also some huge questions around if the config (for the module, not just this PR) is documented correctly or what.

Basically, as I understand it, the conn.create_node call (line 2417 currently) wants ex_labels or ex_tags as keyword arguments. In the code where we get_cloud_config_value we're looking for labels, tags, metadata, etc.

However in the config fixture in the tests we're actually specifying the config values as the kwargs (e.g. ex_labels, ex_tags, etc.) which uh... aren't ever used, so None or whatever the default values are get passed instead. The tests are only passing because the specified config values are completely ignored.

In my branch/for the Test Clinic I went through how to modify the config to require fixtures, as well as add parameters to the fixtures so we can make sure that we're actually providing/checking for the correct values. For each of the def __get_XXX config functions we should be providing:

1. Correct, parseable values (lists, dictionaries, whatever is required)
2. Incorrect, unparseable values (TBH, these functions should probably catch a SyntaxError instead of general Exception)
3. Incorrect, but parseable values: empty data structures, empty strings, incorrect datatypes, e.g. list where a string belongs. (We should also probably be logging a warning when we can't actually parse that data, too, since if it's present then someone would be expecting it to be parsed correctly and not silently just fail).

That interpretation/understanding may be incorrect -- I wasn't actually doing anything with the real connection/library, and I didn't dig into the docs that much.

Personally I think that correct unit tests should be enough here -- I'm pretty sure this is one of the modules that we'll want to extract to an extension module "soon" (for varying sizes of soon) anyway.

@sentient-glare - if my PR/Test Clinic isn't enough to get these tests all sorted out, feel free to reach out to me on IRC (waynew in #salt on Libera.chat), or join Thursday's Test Clinic (or next Tuesday, or whenever you can).

HTH!

[Ch3LL]

bump @sentient-glare did you see @waynew 's comment above ^

[sentient-glare]

Seen! Been on vacation the last little while, looks like I missed the replay but I'll take a look at the branch mentioned.

[sentient-glare]

@waynew @Ch3LL I've gone through and given this an update - some of the __get_x are not currently included in the tests as they are not used in request_instance.

[Ch3LL]

looks like there are test failures related to this change

[sentient-glare]

looks like there are test failures related to this change @Ch3LL

forgot to remove test cases after reverting a previous change, that's been fixed

[Ch3LL]

There is a doc failure:

 Warning, treated as error:
/__w/salt/salt/doc/topics/cloud/gce.rst:241:Title underline too short.

labels
----
[ERROR   ] Unable to load range library

[welcome[bot]]

Congratulations on your first PR being merged! :tada: