MGMT-23822: add unit tests for osac.service roles - #239
openshift-merge-bot[bot] merged 15 commits into
Conversation
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds numerous Ansible test playbooks (unit and integration) for multiple roles: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
collections/ansible_collections/osac/service/roles/lease/tests/test.yml (1)
112-116: Strengthen the nonexistent-release assertion.
that: trueproves only that Ansible reached this task. Consider adding akubernetes.core.k8s_infocheck and asserting the lease count is0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/service/roles/lease/tests/test.yml` around lines 112 - 116, Replace the weak assertion that only proves the task ran by adding a Kubernetes query and a strict zero-count check: after the "Assert no error occurred" task (or in that same conditional block guarded by test_release_nonexistent), run kubernetes.core.k8s_info to fetch Lease resources for the target namespace and then assert the returned resources list length equals 0 (i.e., lease count is 0) so the play verifies no lease exists after attempting to release a nonexistent lease.collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml (1)
41-67: Current assertions are tightly coupled to mutable catalog content.Hard-coding specific IDs (e.g.,
ocp_4_17_small) and requiring'fc430'in every cluster template risks false failures when templates evolve but the role still works.Also applies to: 68-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml` around lines 41 - 67, The tests "Verify cluster templates discovered" and "Verify cluster template structure" are brittle because they hard-code specific template IDs and the node_set key 'fc430'; update them to assert generic, stable properties instead: for the discovery task (osac_cluster_templates) assert the variable is defined and non-empty (length >= 1) rather than requiring particular ids, and remove the superset check against ['osac.templates.ocp_4_17_small', 'osac.templates.ocp_4_17_small_github']; for the structure task (loop over osac_cluster_templates / item) replace the "'fc430' in item.node_sets" check with generic assertions that item.node_sets is defined, is a mapping/list, and contains at least one entry (e.g., item.node_sets | length > 0) while keeping checks for item.id and item.title; keep the same task names and loop_control so changes are localized to those assert blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@collections/ansible_collections/osac/service/roles/cluster_working_namespace/tests/test.yml`:
- Around line 36-70: Wrap the create/run/assert tasks in a single block with the
existing condition (when: test_found | default(false) | bool) and move the
"Cleanup -- delete test namespace" task into that block's always: section so it
runs regardless of failures in the block; keep the cleanup task definition
(kubernetes.core.k8s, state: absent, name: cluster-cwn-test-work) but remove its
standalone when so the block-level when controls whether the whole scenario runs
while the always ensures deletion even on assertion failure.
In `@collections/ansible_collections/osac/service/roles/common/tests/test.yml`:
- Around line 26-67: Add a pre-task validation that ensures the test selector is
configured correctly: assert that exactly one of test_env_set or test_env_unset
is defined and true (and not both false or both true) before including the
get_remote_cluster_kubeconfig role; implement this as an ansible.builtin.assert
task (e.g., named "Validate test selector") placed before the tasks that include
get_remote_cluster_kubeconfig.yaml so the play fails fast if selectors are
missing, misspelled, or misconfigured.
In
`@collections/ansible_collections/osac/service/roles/compute_instance_working_namespace/tests/test.yml`:
- Around line 42-90: Wrap the create/patch/run/verify tasks in a single Ansible
block and move the "Cleanup -- delete test Tenant" task into an always: clause
of a separate block so the deletion runs regardless of failures; keep the when
condition (test_tenant_found | default(false) | bool) applied to the main test
block (not the cleanup) and ensure the cleanup references the same Tenant name
"ciwn-test-tenant" and namespace "default" so the Tenant is removed even if the
assertion in compute_instance_working_namespace fails.
In `@collections/ansible_collections/osac/service/roles/finalizer/tests/test.yml`:
- Around line 37-100: Wrap each test scenario's assertion and its subsequent
cleanup tasks into an Ansible block with an always: section so cleanup runs
regardless of assertion failures; specifically, for the "Verify finalizer was
added" / "Assert finalizer present" and the two "Cleanup -- ..." tasks
(kubernetes.core.k8s_json_patch and kubernetes.core.k8s) that follow (as well as
the other three analogous scenario groups) replace the separate tasks with a
block containing the assert under tasks: and move both cleanup tasks under
always: (or use a block/rescue/always pattern) so the finalizer removal and
ConfigMap deletion execute even when ansible.builtin.assert fails. Ensure you
keep the same vars for finalizer_name/finalizer_target and the when: condition
(test_add_finalizer | default(false) | bool) on the outer block.
In `@collections/ansible_collections/osac/service/roles/lease/tests/test.yml`:
- Around line 39-92: The assertion failure can prevent the "Release lease" task
from running, leaving a Kubernetes Lease behind; wrap the acquire/verify/assert
sequence in an Ansible block and move the "Release lease" and "Verify lease
deleted" tasks into an always: section so the release runs regardless of
assertion errors. Specifically, group the tasks named "Acquire lease", "Verify
lease exists", and "Assert lease created with correct holder and label" into a
block:, then add an always: that contains the existing "Release lease" and
"Verify lease deleted" tasks (and the final "Assert lease gone" if you want to
fail the play on lingering lease); ensure variables like lease_state,
lease_name, lease_holder and registrations (lease_after_acquire /
lease_after_release) remain correctly referenced.
---
Nitpick comments:
In
`@collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml`:
- Around line 41-67: The tests "Verify cluster templates discovered" and "Verify
cluster template structure" are brittle because they hard-code specific template
IDs and the node_set key 'fc430'; update them to assert generic, stable
properties instead: for the discovery task (osac_cluster_templates) assert the
variable is defined and non-empty (length >= 1) rather than requiring particular
ids, and remove the superset check against ['osac.templates.ocp_4_17_small',
'osac.templates.ocp_4_17_small_github']; for the structure task (loop over
osac_cluster_templates / item) replace the "'fc430' in item.node_sets" check
with generic assertions that item.node_sets is defined, is a mapping/list, and
contains at least one entry (e.g., item.node_sets | length > 0) while keeping
checks for item.id and item.title; keep the same task names and loop_control so
changes are localized to those assert blocks.
In `@collections/ansible_collections/osac/service/roles/lease/tests/test.yml`:
- Around line 112-116: Replace the weak assertion that only proves the task ran
by adding a Kubernetes query and a strict zero-count check: after the "Assert no
error occurred" task (or in that same conditional block guarded by
test_release_nonexistent), run kubernetes.core.k8s_info to fetch Lease resources
for the target namespace and then assert the returned resources list length
equals 0 (i.e., lease count is 0) so the play verifies no lease exists after
attempting to release a nonexistent lease.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56880700-39d8-4ee0-9be7-b735dfa14774
📒 Files selected for processing (7)
collections/ansible_collections/osac/service/roles/cluster_working_namespace/tests/test.ymlcollections/ansible_collections/osac/service/roles/common/tests/test.ymlcollections/ansible_collections/osac/service/roles/compute_instance_working_namespace/tests/test.ymlcollections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.ymlcollections/ansible_collections/osac/service/roles/extract_template_info/tests/test.ymlcollections/ansible_collections/osac/service/roles/finalizer/tests/test.ymlcollections/ansible_collections/osac/service/roles/lease/tests/test.yml
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Addressed CodeRabbit's actionable feedback in a46f0a6:
Not changing (intentional design choices):
|
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@zszabo-rh Is there a reason not to add the tier 2 tests to the integration tests? |
|
@trewest I'll convert the Tier 2 tests (finalizer, lease, cluster_working_namespace, compute_instance_working_namespace) to integration test targets and wire them into run_tests.sh. The Tier 1 tests don't need a cluster at all, so they'll stay as role-level unit tests. |
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/run_tests.sh (1)
13-18: Address shellcheck SC2155: separate declaration and assignment.Combining
exportwith command substitution can mask a non-zero exit code fromcat. While the file existence check mitigates this, separating the operations is cleaner.🔧 Suggested fix
# Use real pod UID if available (created by setup_test_env.sh), fall back to placeholder if [ -f "${SCRIPT_DIR}/test-runner-uid" ]; then - export POD_UID=$(cat "${SCRIPT_DIR}/test-runner-uid") + POD_UID=$(cat "${SCRIPT_DIR}/test-runner-uid") + export POD_UID else export POD_UID="00000000-0000-0000-0000-000000000000" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run_tests.sh` around lines 13 - 18, The current script combines export with command substitution when setting POD_UID which triggers shellcheck SC2155; change it to first declare/export the variable (export POD_UID) or assign to a normal variable, then perform the command substitution separately (e.g., read POD_UID_CONTENT="$(cat "${SCRIPT_DIR}/test-runner-uid")" then set POD_UID="$POD_UID_CONTENT" and export), ensuring you still respect the existing if/else check and preserve the fallback value "00000000-0000-0000-0000-000000000000" and references to SCRIPT_DIR and test-runner-uid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/run_tests.sh`:
- Around line 13-18: The current script combines export with command
substitution when setting POD_UID which triggers shellcheck SC2155; change it to
first declare/export the variable (export POD_UID) or assign to a normal
variable, then perform the command substitution separately (e.g., read
POD_UID_CONTENT="$(cat "${SCRIPT_DIR}/test-runner-uid")" then set
POD_UID="$POD_UID_CONTENT" and export), ensuring you still respect the existing
if/else check and preserve the fallback value
"00000000-0000-0000-0000-000000000000" and references to SCRIPT_DIR and
test-runner-uid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f137a2b4-6cfc-4b0d-bdab-43b08b36162b
📒 Files selected for processing (7)
collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.ymltests/integration/run_tests.shtests/integration/setup_test_env.shtests/integration/targets/cluster_working_namespace/tasks/baseline.ymltests/integration/targets/compute_instance_working_namespace/tasks/baseline.ymltests/integration/targets/finalizer/tasks/baseline.ymltests/integration/targets/lease/tasks/baseline.yml
✅ Files skipped from review due to trivial changes (2)
- tests/integration/targets/lease/tasks/baseline.yml
- collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml
Nice! |
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Thanks for adding all these additional tests. I used AI to review and left a suggestion.
|
/hold |
|
@zszabo-rh I intentionally added /hold to this review. I will connect with Trey and you offline on some behavior I've observed. You can /unhold once you are ready to merge. |
Test template discovery from Ansible collections with 4 scenarios: - Happy path: discovers cluster, compute_instance, and network templates from osac.templates with structure validation - Nonexistent collection: returns empty lists gracefully - Invalid collection name format: handled without crash - Mixed valid/invalid collections: discovers from valid, ignores invalid No cluster or external service needed — uses locally installed collections. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test ClusterOrder spec field extraction with 4 scenarios: - Fully qualified template ID with parameters and node requests - Unqualified template ID gets osac.templates. prefix - Pre-defined template_id preserved (role skips extraction) - Empty template parameters parsed as empty dict No cluster needed — pure data extraction from in-memory variables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test get_remote_cluster_kubeconfig task with 2 scenarios:
- Env var set: remote_cluster_kubeconfig populated from process env
- Env var unset: task skipped, variable not defined
Note: lookup('env') reads from controller process environment, not
play-level environment directive. Tests must set env at shell level.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add unit tests for roles that require a Kubernetes cluster (kind is sufficient): finalizer (4 tests): - Add finalizer to resource - Remove finalizer from resource - Add is idempotent (no duplicates) - Remove nonexistent finalizer (no error) lease (2 tests): - Full lifecycle: acquire with holder/label, then release - Release nonexistent lease (no error) Note: requires a real Pod for ownerReference (prevents K8s GC) cluster_working_namespace (3 tests): - Namespace found by ClusterOrder label - Namespace not found (expected failure with block/rescue) - Pre-defined value preserved (role skips lookup) compute_instance_working_namespace (3 tests): - Tenant found with status.namespace set - Tenant not found (expected failure with block/rescue) - Pre-defined value preserved (role skips lookup) Note: uses kubectl --subresource=status for Tenant status patching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ment_specs The osac_network_classes and publish_templates_network_class_api_endpoint options were added to defaults/main.yaml in commit 977c891 but not to argument_specs.yaml. Add them for consistency with the cluster and compute_instance entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Wrap test scenarios in block/always so cleanup runs even when assertions fail (finalizer, lease, cluster_working_namespace, compute_instance_working_namespace) - Replace weak `that: true` assertion in lease release-nonexistent test with actual k8s_info check confirming no lease exists Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move Tier 2 role tests (finalizer, lease, cluster_working_namespace, compute_instance_working_namespace) from role-level tests/ to tests/integration/targets/ and wire into run_tests.sh. Changes: - Create integration test targets with baseline.yml for each role - Add ROLE_TESTS section to run_tests.sh - Create test-runner pod in setup_test_env.sh for lease ownerReference - Use real pod UID (not placeholder) to prevent K8s garbage collection - Address Akshay's review: split fc430 assertion from generic structure validation in enumerate_templates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d38484d to
4869227
Compare
|
@zszabo-rh: This pull request references MGMT-23822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration/run_tests.sh (1)
13-18: Splitexportand assignment to satisfy SC2155.Minor cleanup flagged by shellcheck:
export VAR=$(…)hides the command's exit status fromset -e. Not exploitable here (the-fguard ensurescatsucceeds), but worth fixing for consistency.♻️ Proposed fix
# Use real pod UID if available (created by setup_test_env.sh), fall back to placeholder if [ -f "${SCRIPT_DIR}/test-runner-uid" ]; then - export POD_UID=$(cat "${SCRIPT_DIR}/test-runner-uid") + POD_UID=$(cat "${SCRIPT_DIR}/test-runner-uid") + export POD_UID else export POD_UID="00000000-0000-0000-0000-000000000000" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run_tests.sh` around lines 13 - 18, The current inline export with command substitution for POD_UID should be split to avoid hiding command exit status (SC2155): first assign POD_UID from the file or fallback value using the same condition that checks "${SCRIPT_DIR}/test-runner-uid", then on the next line run export POD_UID; refer to the variables and file names POD_UID, SCRIPT_DIR and test-runner-uid in the change so you update both the true branch (cat "${SCRIPT_DIR}/test-runner-uid") and the else branch (the UUID fallback) to use separate assignment and export statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/setup_test_env.sh`:
- Around line 90-99: The current setup silently hides pod-creation failures by
suppressing kubectl output and writing a zero-UUID into TEST_RUNNER_UID; update
the test-runner creation and UID export flow so failures other than "already
exists" surface and we warn when falling back: remove or limit the stderr
suppression on the kubectl run (or first check existence with kubectl get pod
test-runner), capture and propagate real kubectl errors from the test-runner
creation step, and when you must fall back to the all-zero UUID for
TEST_RUNNER_UID (and write to test-runner-uid in SCRIPT_DIR) emit a clear
warning message indicating the real pod was not created and tests may be
invalid; reference the TEST_RUNNER_UID variable, the kubectl run/get commands
for pod/test-runner, and the output file test-runner-uid in SCRIPT_DIR to locate
where to change behavior.
In
`@tests/integration/targets/compute_instance_working_namespace/tasks/baseline.yml`:
- Around line 70-92: Test 2 is passing accidentally because the fact
compute_instance_working_namespace persists from Test 1 and causes the role
block guarded by "when: compute_instance_working_namespace is not defined" to be
skipped; either reset that fact before Test 2 or make the rescue assertion check
the real role error. Modify the baseline.yml test sequence so before "Run role
with nonexistent tenant reference" you clear/reset the
compute_instance_working_namespace fact (e.g., set it to null/undefined or use
meta: clear_facts) to ensure the role runs, and/or change the assert in the
rescue to match the role's actual error string ("Tenant namespace was not found"
or the exact ansible_failed_result.msg returned by the role) instead of the
generic "'not found' in ansible_failed_result.msg".
In `@tests/integration/targets/finalizer/tasks/baseline.yml`:
- Around line 128-135: The cleanup task "Cleanup -- delete ConfigMap" using
kubernetes.core.k8s with state: absent can hang if the role under test fails to
remove the finalizer; add a preceding task that patches /metadata/finalizers to
[] for the ConfigMap named finalizer-test-remove (using kubernetes.core.k8s or
kubernetes.core.k8s_info/patch) and set failed_when: false so the cleanup won't
block — mirror the same pre-patch pattern used in Tests 1 and 3 to ensure
deletion proceeds even if the finalizer was not removed by the role under test.
---
Nitpick comments:
In `@tests/integration/run_tests.sh`:
- Around line 13-18: The current inline export with command substitution for
POD_UID should be split to avoid hiding command exit status (SC2155): first
assign POD_UID from the file or fallback value using the same condition that
checks "${SCRIPT_DIR}/test-runner-uid", then on the next line run export
POD_UID; refer to the variables and file names POD_UID, SCRIPT_DIR and
test-runner-uid in the change so you update both the true branch (cat
"${SCRIPT_DIR}/test-runner-uid") and the else branch (the UUID fallback) to use
separate assignment and export statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d2c861b-8285-46d2-8dea-40b939ffc4e6
📒 Files selected for processing (10)
collections/ansible_collections/osac/service/roles/common/tests/test.ymlcollections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.ymlcollections/ansible_collections/osac/service/roles/extract_template_info/tests/test.ymlcollections/ansible_collections/osac/service/roles/publish_templates/meta/argument_specs.yamltests/integration/run_tests.shtests/integration/setup_test_env.shtests/integration/targets/cluster_working_namespace/tasks/baseline.ymltests/integration/targets/compute_instance_working_namespace/tasks/baseline.ymltests/integration/targets/finalizer/tasks/baseline.ymltests/integration/targets/lease/tasks/baseline.yml
✅ Files skipped from review due to trivial changes (4)
- collections/ansible_collections/osac/service/roles/publish_templates/meta/argument_specs.yaml
- collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml
- tests/integration/targets/cluster_working_namespace/tasks/baseline.yml
- collections/ansible_collections/osac/service/roles/extract_template_info/tests/test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/targets/lease/tasks/baseline.yml
- Fix fact-persistence bug: Test 2 in cluster_working_namespace and compute_instance_working_namespace passed accidentally because set_fact from Test 1 persisted across plays. Reset facts to null before Test 2 and tighten assertions to match exact role error messages instead of generic substring - Harden setup_test_env.sh: check pod existence before creating, warn clearly on fallback to placeholder UID - Fix SC2155 in run_tests.sh: separate declaration and export - Add safety finalizer removal in finalizer Test 2 cleanup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ansible set_fact persists across plays within a single invocation. Setting a fact to null still counts as "defined", so the role's `when: var is not defined` guard skips the lookup entirely. Fix by reordering plays: "not found" runs first (before any fact exists), "pre-defined" second, "found" last (since it sets the fact via the role). Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Assisted-By: Claude Code <noreply@anthropic.com>
Ansible set_fact persists across plays within a single ansible-playbook invocation, and there is no way to truly undefine a fact (setting to null still counts as "defined"). This caused the "not found" and "found" tests to interfere with each other. Split each scenario into its own file (test_not_found.yml, test_predefined.yml, test_found.yml) and invoke them as separate ansible-playbook calls from run_tests.sh, ensuring complete fact isolation between scenarios. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Assisted-By: Claude Code <noreply@anthropic.com>
|
/retest |
The test-runner pod with a real UID prevented K8s GC of leases, which caused cluster_create:overrides to fail with a 409 Conflict (lease's holderIdentity field owned by a different field manager from the baseline test). Revert to placeholder UID globally -- K8s GC is not instant, so the lease lifecycle test still passes within a single playbook run. Remove test-runner pod creation from setup_test_env.sh since it's no longer needed. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Assisted-By: Claude Code <noreply@anthropic.com>
Create a dedicated lease-test-pod with a real UID only for the role test section of run_tests.sh. This prevents K8s GC from deleting leases during the lease lifecycle test, while keeping the placeholder UID for workflow tests so leases are GC'd between baseline and override runs (fixing the cluster_create:overrides 409 conflict). Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Assisted-By: Claude Code <noreply@anthropic.com>
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, zszabo-rh 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 |
e802b54
into
osac-project:main
4 participants
Summary
osac_network_classesandpublish_templates_network_class_api_endpointin publish_templates argument_specsTier 1: Offline roles (no cluster needed)
enumerate_templatesextract_template_infocommonTier 2: Kind-backed roles (need K8s API, kind cluster sufficient)
finalizerleasecluster_working_namespacecompute_instance_working_namespaceMinor fix: publish_templates argument_specs
Added missing
osac_network_classesandpublish_templates_network_class_api_endpointoptions topublish_templates/meta/argument_specs.yaml. These were added todefaults/main.yamlin commit 977c891 but not to the argument specs. Addresses review feedback from PR #234.Test conventions (matching existing patterns)
roles/<name>/tests/test.yml-e(e.g.,-e test_add_finalizer=true)when: test_var | default(false) | boolblock/rescuewithansible.builtin.assertansible-lintHow to run
Test plan
ansible-lintpasses on all roles (0 failures, 0 warnings)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores