Add unit tests for osism/tasks/openstack.py

[berendt]

Summary

Adds tests/unit/tasks/test_openstack.py with unit tests for the OpenStack Celery worker module, covering the targets listed in #2356:

• Cloud credential/environment helpers: get_cloud_password (hyphen normalization, key-format guard, whitespace stripping, non-string coercion, loader errors), setup_cloud_environment (password-injection branch incl. clouds.yml/missing-auth/missing-profile variants, secure.yml/secure.yaml fallback branch, error paths), cleanup_cloud_environment and get_cloud_helpers.
• Module-level get_openstack_connection(cloud, password): all Unauthorized/MissingRequiredOptions/SDKException exit paths for both the password and the secure.yml attempt, the secure.yml retry after a failed password, and the current_project connection probe. The real keystoneauth1/openstacksdk exception classes are raised.
• Baremetal/NetBox getters: get_baremetal_nodes, get_baremetal_node_ports, get_baremetal_node_netbox_info / get_baremetal_nodes_netbox_info (incl. cf_inventory_hostname fallback), and the secret masking in get_baremetal_node_parameters (value-based masking, ironic_osism_* name-based regex masking after dropped vault decryption, secret_values forwarding into mask_secrets).
• Thin Celery task wrappers: parametrized delegation block plus the specific branches (baremetal_node_create attribute injection, wait_for_nodes_provision_state first/empty result, set_power_state wait/no-wait, generator materialization, set_target_raid_config microversion PUT).
• Manager tasks: image_manager (delegation without configs, temp-dir config rewriting of --images arguments, cleanup on error, documented IndexError for a trailing --images), flavor_manager, project_manager/project_manager_sync, and the task-lock check for all four.

Notes

• The issue hint suggested constructing MissingRequiredOptions with SimpleNamespace(name=...); keystoneauth1 5.15.0 builds the message from o.dest, so the tests use SimpleNamespace(dest=...).
• Instead of patching os.path.exists/os.chdir globally, the tests patch the module-level os/shutil/yaml name bindings of osism.tasks.openstack, so no test can touch the real filesystem or working directory.

Closes #2356

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've left some high level feedback:

• The test_openstack.py module is quite large (~1600 lines) and covers many distinct concerns (cloud env, connections, baremetal, NetBox, managers); consider splitting it into multiple test modules (e.g. test_openstack_env.py, test_openstack_baremetal.py, test_openstack_managers.py) to keep individual files easier to navigate and reason about.
• The tests reach into osism.tasks.conductor.utils for get_vault, deep_decrypt, and mask_secrets while primarily exercising osism.tasks.openstack; if possible, isolating the masking behavior behind a smaller interface in openstack and stubbing that instead would reduce coupling to the conductor implementation details and make the tests more robust to refactors.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `test_openstack.py` module is quite large (~1600 lines) and covers many distinct concerns (cloud env, connections, baremetal, NetBox, managers); consider splitting it into multiple test modules (e.g. `test_openstack_env.py`, `test_openstack_baremetal.py`, `test_openstack_managers.py`) to keep individual files easier to navigate and reason about.
- The tests reach into `osism.tasks.conductor.utils` for `get_vault`, `deep_decrypt`, and `mask_secrets` while primarily exercising `osism.tasks.openstack`; if possible, isolating the masking behavior behind a smaller interface in `openstack` and stubbing that instead would reduce coupling to the conductor implementation details and make the tests more robust to refactors.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[ideaship]

This asserts the OpenStack CLI is still launched when setup_cloud_environment returns cloud_setup_success=False, which locks in a real bug. run_openstack_command_with_cloud unpacks cloud_setup_success (osism/tasks/openstack.py:769) but never checks it, so it always calls run_command(..., ignore_env=True). ignore_env=True launches the CLI with an empty environment — no OS_* variables inherited — so its only credential source is the clouds.yaml/secure.yml files that setup_cloud_environment writes to /tmp on success. When setup failed (e.g. a cloud's password is missing and there's no secure.yml fallback), those files aren't there and the command runs with no credentials instead of aborting. Please fix the short-circuit in its own commit and flip this test to assert run is not called on setup failure. The same unread flag is at :805, :896, :936.

[ideaship]

This asserts an IndexError as the contract, but that crash defeats the very guard it exercises. The configs branch strips any incoming --images so it can force --images <temp_dir> (osism/tasks/openstack.py:817-827); the --images X form pops twice but only guards ValueError, so a trailing bare --images pops past the end and crashes. Please harden the strip in its own commit — pop the value only if one follows — and assert the robust behavior here (trailing --images stripped, --images temp_dir forced, no crash) rather than pinning the crash. (No current caller passes --images into the configs branch, so this path isn't reachable via the CLI today; if the guard is considered unnecessary, removing it and dropping this test is the alternative.)

[ideaship]

This _mask_node_secret_parameters extraction is a production change to the module's surface but ships in the same commit as all the new tests, under an "Add unit tests..." subject. Per our commit-separation convention, please split it into its own commit (accurate subject) ahead of the test-addition commit. The extraction itself looks behavior-preserving.