[edpm_prepare] Apply set_containers overrides from job vars - #4131
Open
rebtoor wants to merge 1 commit into
Open
[edpm_prepare] Apply set_containers overrides from job vars#4131rebtoor wants to merge 1 commit into
rebtoor wants to merge 1 commit into
Conversation
PR Summary by QodoWire set_containers into edpm_prepare and support merge-patch image overrides
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
5 tasks
rebtoor
force-pushed
the
feature/edpm-set-containers-s2i
branch
from
August 21, 2026 08:13
48a3047 to
c05c5c6
Compare
Code Review by Qodo
1. Invalid jsonpath JSON parse
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
rebtoor
changed the title
rebtoor
force-pushed
the
feature/edpm-set-containers-s2i
branch
2 times, most recently
from
August 21, 2026 08:27
9890761 to
699b551
Compare
rebtoor
marked this pull request as draft
August 21, 2026 08:30
rebtoor
force-pushed
the
feature/edpm-set-containers-s2i
branch
from
August 21, 2026 08:31
699b551 to
6edaaf8
Compare
rebtoor
changed the title
rebtoor
force-pushed
the
feature/edpm-set-containers-s2i
branch
2 times, most recently
from
August 21, 2026 10:28
369b1fd to
0ccb20d
Compare
rebtoor
marked this pull request as ready for review
August 21, 2026 13:45
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 32m 12s |
3 tasks
rebtoor
added a commit
to openstack-k8s-operators/watcher-operator
that referenced
this pull request
Aug 21, 2026
Move the speculative watcher consumer off the late pre_tests OpenStackVersion patch and onto cifmw_set_containers_images so images are injected before the control plane deploys. Add opendev-watcher-s2i-pipeline for Gerrit openstack/watcher only; the shared edpm template also covers watcherclient and tempest-plugin. Depends-On: openstack-k8s-operators/s2i-openstack-containers#91 Depends-On: openstack-k8s-operators/ci-framework#4131 Co-authored-by: Cursor <cursoragent@cursor.com>
rebtoor
added a commit
to openstack-k8s-operators/watcher-operator
that referenced
this pull request
Aug 21, 2026
Move the speculative watcher consumer off the late pre_tests OpenStackVersion patch and onto cifmw_set_containers_images so images are injected before the control plane deploys. Fold the s2i provider and deploy jobs into opendev-watcher-edpm-pipeline so RDO config does not need a second template. Limit those jobs to watcher/ source paths so python-watcherclient and watcher-tempest-plugin keep EDPM-only. Depends-On: openstack-k8s-operators/s2i-openstack-containers#91 Depends-On: openstack-k8s-operators/ci-framework#4131 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
recheck |
Contributor
|
/agentic_review |
Comment on lines
+503
to
+506
| "-n", | ||
| params["namespace"], | ||
| "-o", | ||
| "jsonpath={.spec.customContainerImages}", |
There was a problem hiding this comment.
1. Invalid jsonpath json parse 🐞 Bug ≡ Correctness
_fetch_existing_custom_container_images requests jsonpath output but then json.loads() it; jsonpath output is not guaranteed to be valid JSON, so preserve_unlisted can fail with a decode error and block edpm_prepare overrides.
Agent Prompt
### Issue description
`_fetch_existing_custom_container_images()` runs `oc get ... -o jsonpath={.spec.customContainerImages}` and then attempts `json.loads(out)`. JSONPath output is not reliably JSON, so this can raise a decode failure and break the module when `preserve_unlisted=true`.
### Issue Context
The goal of `preserve_unlisted` is to merge the current cluster map with overrides; that requires a reliable, parseable representation of `spec.customContainerImages`.
### Fix Focus Areas
- plugins/modules/set_containers.py[496-524]
### Suggested fix
- Change the `oc get` to `-o json` and `json.loads(out)` the whole object, then extract `obj.get("spec", {}).get("customContainerImages", {})`.
- Alternatively, if available in your target oc/kubectl, use `-o jsonpath-as-json=...`.
- Keep the existing type-check (`dict`) and fail with a clear error if extraction yields a non-mapping.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+510
to
+511
| if rc != 0 or not out.strip(): | ||
| return {} |
There was a problem hiding this comment.
2. Preserve ignores oc get failures 🐞 Bug ☼ Reliability
When preserve_unlisted is enabled, oc get failures (rc!=0) return an empty dict, so the module may apply only the override list and unintentionally drop existing customContainerImages entries.
Agent Prompt
### Issue description
With `preserve_unlisted=true`, `_fetch_existing_custom_container_images()` returns `{}` on any `oc get` non-zero rc, silently disabling preservation and risking loss of existing image mappings.
### Issue Context
The PR’s design states partial override lists should keep unspecified images from the current OpenStackVersion CR. Returning `{}` on errors violates that guarantee.
### Fix Focus Areas
- plugins/modules/set_containers.py[496-524]
- plugins/modules/set_containers.py[635-643]
### Suggested fix
- If `rc != 0`, call `module.fail_json(msg=..., rc=rc, stdout=out, stderr=err)`.
- If you need to tolerate “not found” (CR absent), detect that specific case (e.g., parse `err` for NotFound / check `out`), and only then return `{}`.
- Consider also failing if `out.strip()` is empty while `rc==0` (unexpected) unless you can prove it represents an empty map.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+635
to
+639
| if module.params["preserve_unlisted"]: | ||
| if not kubeconfig: | ||
| module.fail_json(msg="kubeconfig is required when preserve_unlisted=true") | ||
| existing_images = _fetch_existing_custom_container_images( | ||
| module, module.params, kubeconfig |
There was a problem hiding this comment.
3. Check mode hits cluster 🐞 Bug ☼ Reliability
preserve_unlisted triggers an oc get even in Ansible check mode, so set_containers no longer behaves as a pure local diff in check mode and may fail in environments where cluster access isn’t available.
Agent Prompt
### Issue description
The module advertises `supports_check_mode=True`, but the new `preserve_unlisted` branch performs a live `oc get` regardless of check mode.
### Issue Context
Check mode is commonly used for dry-run validation and should avoid side-effecting operations and ideally avoid requiring external connectivity.
### Fix Focus Areas
- plugins/modules/set_containers.py[635-643]
### Suggested fix
- Add a guard:
- If `module.check_mode` and `preserve_unlisted` is true, either:
- skip the cluster fetch and document that preservation cannot be evaluated in check mode, or
- read preservation source from an existing on-disk manifest if present.
- If you choose to skip, consider setting a result field/warning to make behavior explicit.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+338
to
+363
| def test_present_preserve_unlisted_calls_oc_get_and_apply(self): | ||
| set_module_args( | ||
| self._args( | ||
| apply=True, | ||
| preserve_unlisted=True, | ||
| kubeconfig="/home/zuul/.kube/config", | ||
| images=[ | ||
| dict( | ||
| name="glanceAPIImage", | ||
| full_registry="registry.example:5000/ns/glance:tag", | ||
| ) | ||
| ], | ||
| ) | ||
| ) | ||
| with patch(_MOD + ".os.path.exists", return_value=False), patch( | ||
| _MOD + ".os.makedirs" | ||
| ), patch("builtins.open", mock_open()), patch( | ||
| _MOD + "._run_oc", | ||
| side_effect=[(0, '{"keystoneAPIImage": "keep-me"}', ""), (0, "", "")], | ||
| ) as mock_oc: | ||
| with self.assertRaises(AnsibleExitJson) as cm: | ||
| set_containers.run_module() | ||
| self.assertEqual(mock_oc.call_count, 2) | ||
| self.assertEqual(mock_oc.call_args_list[0][0][1][0], "get") | ||
| self.assertEqual(mock_oc.call_args_list[1][0][1][0], "apply") | ||
| self.assertTrue(cm.exception.args[0]["changed"]) |
There was a problem hiding this comment.
4. Test doesn't validate merge 🐞 Bug ⚙ Maintainability
The new preserve_unlisted unit test only asserts that oc get/apply are called, but never verifies that the written CR actually retains the existing image plus the override, so regressions in merge behavior can slip through.
Agent Prompt
### Issue description
`test_present_preserve_unlisted_calls_oc_get_and_apply` does not assert the merged `customContainerImages` content written to `dest_path`, so it doesn't actually test the preservation behavior.
### Issue Context
The PR’s key behavior is merging existing images with overrides; validating file content would catch breakages in parsing/merge logic.
### Fix Focus Areas
- tests/unit/modules/test_set_containers.py[338-363]
### Suggested fix
- Capture the file write from `mock_open()` and parse the YAML written.
- Assert that `spec.customContainerImages` contains both:
- the preserved key from the mocked oc get output, and
- the overridden key from the input `images` list.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 0ccb20d |
Add a small edpm_prepare task that calls cifmw.general.set_containers when jobs provide cifmw_set_containers_images. The legacy update_containers role is unchanged. Add preserve_unlisted to set_containers so partial override lists keep unspecified images from the current OpenStackVersion CR before apply. Fetch the current CR with oc get -o json and fail on non-NotFound lookup errors so preservation cannot silently drop existing images. Assisted-By: Cursor Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Roberto Alfieri <ralfieri@redhat.com>
rebtoor
force-pushed
the
feature/edpm-set-containers-s2i
branch
from
August 24, 2026 09:53
0ccb20d to
749545e
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
Contributor
Author
|
Addressed the Qodo comments that still applied after the
Left as-is: check-mode still does a read-only |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evallesp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Valkyrie00
approved these changes
Aug 24, 2026
3 tasks
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 10m 17s |
rebtoor
added a commit
to openstack-k8s-operators/watcher-operator
that referenced
this pull request
Aug 24, 2026
Inject s2i-built watcher images through cifmw_set_containers_images during edpm_prepare so they land in OpenStackVersion before the control plane deploys. github-check uses the non-meta operator content provider for operator images and appends the s2i registry as an extra insecure registry so CRC can pull both the operator catalog and watcher service images. Fold the s2i provider and deploy jobs into opendev-watcher-edpm-pipeline and limit them to watcher/ source paths so python-watcherclient and watcher-tempest-plugin keep EDPM-only. Depends-On: openstack-k8s-operators/s2i-openstack-containers#91 Depends-On: openstack-k8s-operators/ci-framework#4131 Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
edpm_preparethat callscifmw.general.set_containerswhen jobs providecifmw_set_containers_images.update_containersrole untouched.preserve_unlistedtoset_containersso partial override lists keep unspecified images from the current OpenStackVersion CR before apply.Design
Problem
Consumer jobs (for example the s2i content provider) return a partial
customContainerImagesmap: only the services that were built, not the fullOpenStack image set. Those overrides must be applied during
edpm_prepare,before
OpenStackControlPlaneis deployed. Applying them later (for examplevia a
pre_testshook) is too late — the control plane and dataplane mayalready be running with operator-default images.
Approach
cifmw_set_containers_imagesas a list of{name, full_registry}entries.edpm_prepareruns one genericset_containerstask when that list is non-empty.preserve_unlistedreads the current OpenStackVersion CR, keeps entries notpresent in the override list, overlays the new entries, writes the manifest,
and applies it via the module's existing
oc apply -fpath.from provider output to
cifmw_set_containers_images.Out of scope (follow-up)
update_containersrole toset_containersfor allEDPM and DLRN flows. The architecture deploy path already uses
set_containers; this change adds a narrow override hook for EDPM jobs thatsupply explicit image lists.
update_containerswholesale in this PR would duplicate the largeparameter surface already present in
deploy_architecture.ymland expandreview scope beyond what consumer jobs need today.
Alternatives considered
pre_testshook +oc patch: wrong point in the deploy timeline.set_containerscall inedpm_prepare: rejected; jobvars plus a thin task keeps the framework generic.
update_containerstoset_containersin this PR: largerchange, separate from the partial-override use case.
preserve_unlisted: would replace theentire
customContainerImagesmap and drop unspecified services.Test plan
make ansible_test(set_containers unit tests)Depends-Onfrom s2i-openstack-containers PR Bring openstack-tempest-skiplist changes while preserving history #91Depends-on: #4136