Skip to content

Add unit tests for osism/tasks/openstack.py#2459

Open
berendt wants to merge 1 commit into
mainfrom
unit-tests-tasks-openstack
Open

Add unit tests for osism/tasks/openstack.py#2459
berendt wants to merge 1 commit into
mainfrom
unit-tests-tasks-openstack

Conversation

@berendt

@berendt berendt commented Jul 13, 2026

Copy link
Copy Markdown
Member

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

Cover the cloud credential/environment helpers (get_cloud_password,
setup_cloud_environment, cleanup_cloud_environment and the module-level
get_openstack_connection), the baremetal/NetBox getters including the
secret masking in get_baremetal_node_parameters, the thin Celery task
wrappers around the OpenStack SDK, and the manager tasks
(image_manager, flavor_manager, project_manager/project_manager_sync).

All external effects are mocked: the SDK connection comes from a
patched osism.utils.get_openstack_connection, the NetBox client
replaces the lazy osism.utils.nb attribute, and the environment
helpers never touch the real filesystem or working directory because
the module-level os/shutil/yaml name bindings (plus builtins.open) are
patched. keystoneauth1 and openstacksdk are pinned runtime
dependencies, so their real exception classes are raised instead of
stand-ins.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

Unit tests for osism/tasks/openstack.py

2 participants