[deploy_cr] - Support for configurable conditions and defaults #725
Conversation
📝 WalkthroughWalkthroughThis update adds a detailed README for the Changes
Possibly related PRs
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
betoredhat
force-pushed
the
update_deploy_cr
branch
from
04585c9 to
80564db
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 3m 21s |
|
from change https://github.com/dci-labs/bos2-ci-config/pull/389:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 54s |
betoredhat
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
roles/deploy_cr/defaults/main.yaml (1)
7-7:dc_wait_for_crdefault conflicts with READMEDefaults set it to
true, README says the default isfalse.
Please align one of them (and mention in the changelog).-dc_wait_for_cr: true +# Most users expect “fire-and-forget”; make this explicit. +dc_wait_for_cr: falseroles/deploy_cr/README.md (1)
18-18: Update default value in the docsRow shows “Defaults to
false” butdefaults/main.yamlcurrently setstrue.
Once the default is finalised, reflect it here to keep docs trustworthy.roles/deploy_cr/tasks/main.yml (2)
4-13: Header comment still references legacy variable namesThe comment block lists
api_version,kind, etc., but the tasks now expectdc_api_version,dc_kind, …
Please update the doc-string to avoid confusing future readers.
29-29: Drop thetest_prefix in the task name
test_ Check that the CR is readylooks accidental and shows up verbatim in logs/JUnit reports.
Consider a plain, human-friendly name, e.g.Check that the CR is ready.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
roles/deploy_cr/README.md(1 hunks)roles/deploy_cr/defaults/main.yaml(1 hunks)roles/deploy_cr/tasks/main.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
roles/deploy_cr/README.md (1)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
roles/deploy_cr/defaults/main.yaml (1)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
roles/deploy_cr/tasks/main.yml (2)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
🪛 markdownlint-cli2 (0.17.2)
roles/deploy_cr/README.md
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (2)
roles/deploy_cr/README.md (1)
34-36: Example uses correct keys, but they differ from defaultsThe example lists:
wait_condition: type: Available status: "True"Good – these are the proper keys. After fixing the defaults file, everything will be consistent.
roles/deploy_cr/tasks/main.yml (1)
36-38: Relies on correctedwait_conditionkeysOnce the defaults file is fixed to use
type/status, this task will behave as intended.
No action needed here after that fix.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 56s |
tonyskapunk
left a comment
tonyskapunk
left a comment
There was a problem hiding this comment.
There is still no argument_specs, maybe missed to push it?
This change requires an increase in the version as it breaks compatibility with the previous way of use.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 57s |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
roles/deploy_cr/README.md (1)
7-20: Fix malformed Markdown table & incorrect descriptionsThe variable table has ❌ mis-aligned columns and a duplicated “Required” field on
dc_wait_mc_delay.
Besides breaking the rendered table, the current text says “Retries” for a “delay” variable.-| Variable | Required | Type | Description | -|--------------------|----------|--------|---------------------------------------------------------------------------------------------------------| +| Variable | Required | Type | Description | +|--------------------|:--------:|------|-------------| ... -| dc_wait_mc_retries | no | int | Retries for changes that may trigger MCP updates. Defaults to 600 | -| dc_wait_mc_delay |no | no | int | Retries for changes that may trigger MCP updates. Defaults to 10 | +| dc_wait_mc_retries | no | int | Number of retries while waiting for MachineConfigPool updates. Defaults to `600`. | +| dc_wait_mc_delay | no | int | Delay **in seconds** between MCP retries. Defaults to `10`. |This also resolves the markdown-lint errors (MD055, MD056, MD007).
roles/deploy_cr/tasks/main.yml (1)
31-41: Always waiting 60 s + MCP checks hurts play speedA fixed pause plus an MCP poll runs even when the CR has nothing to do with MachineConfigPools. Recommend protecting both tasks with a toggle, e.g.:
vars: dc_wait_for_mcp: false # default ... - name: Pause to wait for MCP to be updated if any when: dc_wait_for_mcp | bool ... - name: Wait for MCPs changes applied if any when: dc_wait_for_mcp | boolThis keeps fast feedback for roles that don’t touch MCPs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ansible-collection-redhatci-ocp.spec(2 hunks)roles/deploy_cr/README.md(1 hunks)roles/deploy_cr/defaults/main.yaml(1 hunks)roles/deploy_cr/meta/argument_specs.yml(1 hunks)roles/deploy_cr/tasks/main.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- roles/deploy_cr/meta/argument_specs.yml
- ansible-collection-redhatci-ocp.spec
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
roles/deploy_cr/defaults/main.yaml (2)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
roles/deploy_cr/tasks/main.yml (3)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#715
File: roles/copy_and_render/tasks/main.yml:18-24
Timestamp: 2025-06-24T21:40:17.028Z
Learning: The copy_and_render role in the ansible-collection-redhatci-ocp repository is designed to work only with relative paths for car_source_dir, not absolute paths. This is an intentional design constraint.
roles/deploy_cr/README.md (1)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
🪛 markdownlint-cli2 (0.17.2)
roles/deploy_cr/README.md
8-8: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
10-10: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
11-11: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
13-13: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
14-14: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
15-15: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
16-16: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
17-17: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
18-18: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
19-19: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
20-20: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
20-20: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.18)
🔇 Additional comments (2)
roles/deploy_cr/README.md (1)
70-71: Example deploys an invalid cluster-scoped resourcePassing
namespace: "omit"will still serialise the field and the API will reject cluster-scoped CRDs.
Either:
- Remove the key entirely in the example, or
- Add wording that the role must be called with
dc_namespaceunset/empty and update the implementation to dropmetadata.namespacewhen blank (see comment intasks/main.yml).Please adjust to prevent copy-paste failures.
roles/deploy_cr/defaults/main.yaml (1)
5-8: Defaultdc_wait_timeout(10 s) is probably too small
kubernetes.core.k8s_infofirst waits up todc_wait_timeout, then the task retries up todc_wait_retries.
With a 10-second inner timeout you’ll loop needlessly for resources that normally take minutes. Consider something like 300 s to reduce API chatter.No change required if the intent is deliberate.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 51s |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 3m 03s |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 53s |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
roles/deploy_cr/meta/argument_specs.yml (1)
20-24: Refinedc_specvalidation for better UX.
dc_specis typed as a nakeddict, so any structure slips through until the API server rejects it.
If the role expects a well-known schema (e.g.,spec.replicas,spec.image), consider addingsuboptionsto surface friendly errors early.
If the spec is intentionally free-form, a brief note in the description (“free-form CR spec; not validated by the role”) would avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
roles/deploy_cr/defaults/main.yaml(1 hunks)roles/deploy_cr/meta/argument_specs.yml(1 hunks)roles/deploy_cr/tasks/main.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- roles/deploy_cr/tasks/main.yml
- roles/deploy_cr/defaults/main.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
roles/deploy_cr/meta/argument_specs.yml (1)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Ansible-lint Check
| options: | ||
| dc_api_version: | ||
| type: str | ||
| required: true | ||
| description: > | ||
| The API version of the CR. | ||
| dc_kind: | ||
| type: str | ||
| required: true | ||
| description: > | ||
| The kind of the CR. | ||
| dc_name: | ||
| type: str | ||
| required: true | ||
| description: > | ||
| The name of the CR. | ||
| dc_spec: | ||
| type: dict | ||
| required: true | ||
| description: > | ||
| The spec of the CR. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add the optional dc_* variables that the role already consumes.
The tasks/README mention dc_namespace, dc_wait_for_ready, dc_wait_for_conditions, dc_wait_retries, etc., yet they’re absent here.
Ommiting them means:
- Users can’t discover them via
ansible-doc. - Collection-level argument validation won’t catch typos (
dc_namepsacewould silently pass).
A minimal patch:
dc_namespace:
type: str
+ default: "default"
description: >
Namespace where the CR should be created.
+ dc_wait_for_ready:
+ type: bool
+ default: true
+ description: >
+ Whether to wait for the CR to reach Ready=True.
+ dc_wait_for_conditions:
+ type: list
+ elements: str
+ default: []
+ description: >
+ Additional status.conditions to wait for.
+ dc_wait_retries:
+ type: int
+ default: 30
+ description: >
+ How many readiness checks to attempt before failing.This keeps argument specs and implementation in sync.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In roles/deploy_cr/meta/argument_specs.yml around lines 4 to 24, add the missing
optional dc_* variables such as dc_namespace, dc_wait_for_ready,
dc_wait_for_conditions, and dc_wait_retries that the role already uses. Define
their types, set required to false, and provide appropriate descriptions. This
will ensure these variables appear in ansible-doc and enable argument validation
to catch typos and improve user discoverability.
98bd77c to
fbcae78
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 49s |
tonyskapunk
left a comment
tonyskapunk
left a comment
There was a problem hiding this comment.
LGTM, please squash your commits before merging.
betoredhat
force-pushed
the
update_deploy_cr
branch
from
fbcae78 to
9c0f6ab
Compare
|
recheck |
|
from change #725:
|
1 similar comment
|
from change #725:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 49s |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
roles/deploy_cr/README.md (1)
7-21: Fix duplicated wording & table lint issuesRow 20 (
dc_wait_mc_delay) repeats the “Retries” wording from the previous row, but the var is a delay in seconds.
While touching the table, you can also drop the trailing pipes somarkdownlint(MD055) stops complaining.-| dc_wait_mc_delay | no | int | Retries for changes that may trigger MCP updates. Defaults to 10 | +| dc_wait_mc_delay | no | int | Delay (seconds) between MCP readiness checks. Defaults to `10`. |[nitpick]
roles/deploy_cr/tasks/main.yml (1)
31-42: Avoid unconditional 60 s pause & MCP pollingThe role always sleeps and polls MachineConfigPools, even when the CR couldn’t influence them (e.g., HyperConverged with no MCP touch). This adds at least one minute to every run.
Guard these tasks, e.g.:
- block: - ansible.builtin.pause: seconds: 60 - name: Wait for MCPs changes applied if any ansible.builtin.include_role: name: redhatci.ocp.check_resource vars: resource_to_check: "MachineConfigPool" check_wait_retries: "{{ dc_wait_mc_retries }}" check_wait_delay: "{{ dc_wait_mc_delay }}" when: - _deploy_cr_create_result.changed - dc_wait_mc_retries | int > 0Keeps fast paths fast while preserving the safeguard.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ansible-collection-redhatci-ocp.spec(2 hunks)galaxy.yml(1 hunks)roles/deploy_cr/README.md(1 hunks)roles/deploy_cr/defaults/main.yaml(1 hunks)roles/deploy_cr/meta/argument_specs.yml(1 hunks)roles/deploy_cr/tasks/main.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- galaxy.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- ansible-collection-redhatci-ocp.spec
- roles/deploy_cr/meta/argument_specs.yml
- roles/deploy_cr/defaults/main.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
roles/deploy_cr/tasks/main.yml (3)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#651
File: roles/odf_setup/tasks/tests.yml:2-2
Timestamp: 2025-05-22T18:25:16.134Z
Learning: When working with the redhatci-ocp collection, task names that will appear in JUnit results should use underscores consistently without spaces (e.g., "Test_CephRBD_PVC" instead of "Test_ CephRBD PVC") to ensure compatibility with the dcijunit callback that generates JUnit reports.
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#715
File: roles/copy_and_render/tasks/main.yml:18-24
Timestamp: 2025-06-24T21:40:17.028Z
Learning: The copy_and_render role in the ansible-collection-redhatci-ocp repository is designed to work only with relative paths for car_source_dir, not absolute paths. This is an intentional design constraint.
roles/deploy_cr/README.md (1)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
🪛 markdownlint-cli2 (0.17.2)
roles/deploy_cr/README.md
8-8: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
10-10: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
11-11: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
13-13: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
14-14: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
15-15: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
16-16: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
17-17: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
18-18: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
19-19: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
20-20: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
Co-authored-by: Tony Garcia <[email protected]>
|
from change #725:
|
1 similar comment
|
from change #725:
|
Co-authored-by: Tony Garcia <[email protected]>
|
from change #725:
|
1 similar comment
|
from change #725:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 49s |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
roles/deploy_cr/README.md (2)
19-20: Copy-paste mix-up between “retries” and “delay”.The description for
dc_wait_mc_delayerroneously repeats the “Retries” wording instead of “Delay”, which can confuse consumers.-| dc_wait_mc_retries | no | int | Retries for changes that may trigger MCP updates. Defaults to: 600 | -| dc_wait_mc_delay | no | int | Retries for changes that may trigger MCP updates. Defaults to 10 | +| dc_wait_mc_retries | no | int | Retries for MCP updates. Defaults to **600**. | +| dc_wait_mc_delay | no | int | Delay (seconds) between MCP retries. Defaults to **10**. |
7-20: Resolve markdownlint table warnings (trailing pipes).Static analysis flags every row because of the trailing
|. Dropping the final pipe aligns withMD055and keeps the table renderer-friendly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
roles/deploy_cr/README.md(1 hunks)roles/deploy_cr/meta/argument_specs.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- roles/deploy_cr/meta/argument_specs.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#725
File: roles/deploy_cr/tasks/main.yml:7-10
Timestamp: 2025-07-15T23:08:36.282Z
Learning: In Ansible, the `default(omit)` filter only works when a variable is undefined, not when it's set to the string "omit". For the deploy_cr role in ansible-collection-redhatci-ocp, cluster-scoped resources should not define the dc_namespace variable at all, rather than setting it to the string "omit".
roles/deploy_cr/README.md (2)
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#652
File: roles/example_cnf_deploy/scripts/get-example-cnf-status.sh:45-45
Timestamp: 2025-04-21T23:02:58.178Z
Learning: When deprecating components in the redhatci/ansible-collection-redhatci-ocp repository, references to the deprecated component in other roles (like references to `cnf-app-mac-operator` in `fbc_catalog`) may be left intact if they serve as examples rather than functional components. Deprecation scope should be limited to the directly affected role (in this case `example_cnf_deploy`).
Learnt from: tonyskapunk
PR: redhatci/ansible-collection-redhatci-ocp#725
File: roles/deploy_cr/tasks/main.yml:7-10
Timestamp: 2025-07-15T23:08:36.282Z
Learning: In Ansible, the `default(omit)` filter only works when a variable is undefined, not when it's set to the string "omit". For the deploy_cr role in ansible-collection-redhatci-ocp, cluster-scoped resources should not define the dc_namespace variable at all, rather than setting it to the string "omit".
🪛 markdownlint-cli2 (0.17.2)
roles/deploy_cr/README.md
8-8: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
10-10: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
11-11: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
13-13: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
14-14: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
15-15: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
16-16: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
17-17: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
18-18: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
19-19: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
20-20: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Sanity Check (stable-2.18)
| dc_api_version: "{{ kubelet_config_cr.api_version }}" | ||
| dc_kind: "{{ kubelet_config_cr.kind }}" | ||
| dc_namespace: "{{ kubelet_config_cr.namespace }}" | ||
| dc_name: "{{ kubelet_config_cr.name }}" | ||
| dc_spec: "{{ kubelet_config_cr.spec }}" |
There was a problem hiding this comment.
Passing an undefined variable will raise an error in the second example.
kubelet_config_cr does not define namespace, so dc_namespace: "{{ kubelet_config_cr.namespace }}" will evaluate to undefined and Ansible will fail before the default(omit) filter in the role can help. Omit the parameter entirely to showcase correct cluster-scoped usage.
- dc_namespace: "{{ kubelet_config_cr.namespace }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dc_api_version: "{{ kubelet_config_cr.api_version }}" | |
| dc_kind: "{{ kubelet_config_cr.kind }}" | |
| dc_namespace: "{{ kubelet_config_cr.namespace }}" | |
| dc_name: "{{ kubelet_config_cr.name }}" | |
| dc_spec: "{{ kubelet_config_cr.spec }}" | |
| dc_api_version: "{{ kubelet_config_cr.api_version }}" | |
| dc_kind: "{{ kubelet_config_cr.kind }}" | |
| dc_name: "{{ kubelet_config_cr.name }}" | |
| dc_spec: "{{ kubelet_config_cr.spec }}" |
🤖 Prompt for AI Agents
In roles/deploy_cr/README.md around lines 89 to 93, the variable dc_namespace is
assigned from kubelet_config_cr.namespace, which is undefined and causes an
error in Ansible. To fix this, remove the line setting dc_namespace entirely to
demonstrate correct cluster-scoped usage without passing an undefined variable.
| |--------------------|----------|--------|---------------------------------------------------------------------------------------------------------| | ||
| | dc_api_version | yes | str | The API version of the CR (e.g., `hco.kubevirt.io/v1beta1`). | | ||
| | dc_kind | yes | str | The kind of the CR (e.g., `HyperConverged`). | | ||
| | dc_namespace | no | str | The namespace in which the CR should be created. Defaults to `omit` | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify namespace default – avoid suggesting the literal string “omit”.
Per upstream task logic ( default(omit) ) the variable should be unset for cluster-scoped resources. Documenting the default as the string omit is misleading and will break the filter-based omission pattern.
-| dc_namespace | no | str | The namespace in which the CR should be created. Defaults to `omit` |
+| dc_namespace | no | str | Namespace for the CR. Leave **undefined** for cluster-scoped objects. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | dc_namespace | no | str | The namespace in which the CR should be created. Defaults to `omit` | | |
| | dc_namespace | no | str | Namespace for the CR. Leave **undefined** for cluster-scoped objects. | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
In roles/deploy_cr/README.md at line 11, the default value for dc_namespace is
incorrectly documented as the literal string "omit". Update the documentation to
clarify that the default behavior is to leave the variable unset for
cluster-scoped resources, rather than setting it to the string "omit", to
accurately reflect the upstream task logic and avoid confusion.
SUMMARY
Some updates to the role to allow other wait conditions, defaults, readme, etc
Test-Hint: no-check