[set_containers] Add cifmw.general.set_containers module#3955
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 18m 45s |
arxcruz
force-pushed
the
feature/set-containers-module
branch
from
4b08e81 to
7ff95ec
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 26m 34s |
arxcruz
force-pushed
the
feature/set-containers-module
branch
4 times, most recently
from
507f3c3 to
20386f6
Compare
Replace the update_containers role invocation in deploy_architecture with the new cifmw.general.set_containers module. The module generates and optionally applies an OpenStackVersion CR, accepting a dynamic images list where each entry can override registry, org, name_prefix, and tag individually. This removes the rigid Jinja2 template in favor of a Python module that is easier to test, extend, and call from any playbook without role-level variable coupling. Key design points: - _OPENSTACK_SUFFIXES table enables partial overrides (e.g. tag-only) for any standard OpenStack image field without repeating the suffix. - Empty name_prefix is supported so images that ship without the "openstack-" prefix (EDPM, IPA) can be built from parts rather than requiring a pre-assembled full_registry URL. - backends list handles cinderVolumeImages / manilaShareImages nested dicts. - Unit tests cover _build_url, _resolve_image, _resolve_backend, _build_cr, and the full run_module flow including apply, absent, and validation paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Arx Cruz <arxcruz@redhat.com>
arxcruz
force-pushed
the
feature/set-containers-module
branch
from
20386f6 to
069b3b9
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 21m 40s |
jokke-ilujo
left a comment
jokke-ilujo
left a comment
There was a problem hiding this comment.
I have some concerns of the dest_path file handling. Other than that looks quite good on the first pass.
| module.fail_json( | ||
| msg="oc delete failed", rc=rc, stdout=out, stderr=err | ||
| ) | ||
| os.remove(dest_path) |
There was a problem hiding this comment.
As the dest_path is a freeform input from the module parameters without sanitization this module can behave like universal delete and we only check that the path exists. We should probably encapsulate the os.remove in try-except block for various error conditions and fail the module gracefully should they occur.
| cr_yaml = yaml.dump(cr, default_flow_style=False, sort_keys=False) | ||
|
|
||
| existing = None | ||
| if os.path.exists(dest_path): |
There was a problem hiding this comment.
This does not guarantee That the dest_path is a file nor that we actually can read it (it might be for example existing folder returning true). os.path.isabs and os.path.is_file checks with graceful module failure could mitigate the risks here quite a bit.
| kubeconfig=dict(type="path", default=None), | ||
| ) | ||
|
|
||
| module = AnsibleModule(argument_spec=module_args, supports_check_mode=True) |
There was a problem hiding this comment.
Using ansible.module_utils.common.arg_spec.ArgumentSpecValidator here could be very useful to ensure we actually have correct looking arguments provided before moving forward.
| existing = None | ||
| if os.path.exists(dest_path): | ||
| with open(dest_path, "r") as fh: | ||
| existing = fh.read() |
There was a problem hiding this comment.
We probably should check here that the "existing" somewhat looks like CR, start by yaml parsing "is this file content actually yaml", etc. To make sure we don't overwrite some random file in the filesystem with the new CR.
| dest_dir = os.path.dirname(dest_path) | ||
| if dest_dir: | ||
| os.makedirs(dest_dir, exist_ok=True) | ||
| with open(dest_path, "w") as fh: |
There was a problem hiding this comment.
Ditto with read. We have no guarantees at this point that the dest_path is a writeable file. We should try and fail gracefully.
| set_containers.run_module() | ||
| mock_rm.assert_called_once_with("/tmp/set_containers.yml") | ||
| self.assertTrue(cm.exception.args[0]["changed"]) | ||
|
|
There was a problem hiding this comment.
Add tests for various invalid 'dest_path's (no write permissions, directory, empty string "", invalid content (say /etc/hosts for example))
3 participants
Replace the update_containers role invocation in deploy_architecture with the new cifmw.general.set_containers module. The module generates and optionally applies an OpenStackVersion CR, accepting a dynamic images list where each entry can override registry, org, name_prefix, and tag individually. This removes the rigid Jinja2 template in favor of a Python module that is easier to test, extend, and call from any playbook without role-level variable coupling.
Key design points: