[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
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.
| 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.
| 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))
arxcruz
force-pushed
the
feature/set-containers-module
branch
from
069b3b9 to
cc1fa8f
Compare
| module.fail_json( | ||
| msg="dest_path '{}' contains invalid YAML: {}".format(dest_path, exc) | ||
| ) | ||
|
|
There was a problem hiding this comment.
As it stands now should also except at least IsADirectoryError, PermissionError and IOError; or OSError if we don't want to be descriptive of the nature of the error of which caused the open() or read() to fail.
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 42m 22s |
| # cifmw_update_containers_watcher -> cifmw_set_containers_extra_images | ||
| # cifmw_update_containers_edpm_image_url -> cifmw_set_containers_images (name_prefix: "") | ||
| # cifmw_update_containers_ipa_image_url -> cifmw_set_containers_images (name_prefix: "") | ||
| registry: "{{ cifmw_set_containers_registry | default(cifmw_update_containers_registry | default(omit)) }}" |
There was a problem hiding this comment.
(blocking) possible error: I think this should be: {{ cifmw_set_containers_registry | default(cifmw_update_containers_registry) | default(omit) }}
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
cc1fa8f to
ed106cb
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 27m 03s |
4 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: