Re-run acceptance CA cert; move condition to main task#217
Merged
Conversation
|
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:
WalkthroughReplaces per-item CSR approval with a batched shell task that lists Pending CSRs and pipes their names to Changes
Sequence Diagram(s)sequenceDiagram
participant Ansible
participant Shell
participant OC_CLI as "oc CLI"
participant KubeAPI as "Kubernetes API"
Ansible->>Shell: run shell task to list Pending CSRs
Shell->>OC_CLI: pass CSR names via xargs to approve
OC_CLI->>KubeAPI: POST approvals for CSRs
KubeAPI-->>OC_CLI: return approval responses
OC_CLI-->>Shell: exit status
alt non-zero exit (retryable)
Shell-->>Ansible: failure
Ansible->>Shell: retry after delay (up to N times)
else zero exit
Shell-->>Ansible: success
Ansible->>OC_CLI: apply /tmp/cluster-cert-approver.yaml
OC_CLI->>KubeAPI: create/apply resources
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml`:
- Around line 22-24: The task currently uses an impossible retry condition
"until: false" which makes it always fail; change the task to register its
command result (e.g., add "register: accept_result") and replace "until: false"
with a check on that register such as "until: accept_result.rc == 0" (or a
specific stdout check like "until: "'approved' in accept_result.stdout" if you
expect text); ensure the task keeps "retries" and "delay" so it will retry until
the registered condition becomes true.
danpawlik
force-pushed
the
retry-command
branch
3 times, most recently
from
abe0a58 to
03cfbed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml`:
- Around line 2-9: The loop that runs the shell task which approves Pending CSRs
(the task starting "Re-run accept certificates in Pending state few times") will
stop on the first failure and skip further iterations; update the task to either
add "ignore_errors: true" and "changed_when: false" so all three iterations run
regardless of non-zero exit codes and approvals are treated idempotently, or
replace the loop-based retry with Ansible's retry idiom (use retries/delay/until
on the oc get/oc adm approve command) to implement true retry semantics.
03cfbed to
9b0c996
Compare
danpawlik
changed the title
danpawlik
force-pushed
the
retry-command
branch
from
9b0c996 to
3050619
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml`:
- Around line 3-6: The shell pipeline in the ansible task (ansible.builtin.shell
executing the oc/awk/xargs sequence) can hide failures; update the task to run
under /bin/bash and enable pipefail so any command in the pipeline failing
causes the task to fail: set executable to /bin/bash (executable: /bin/bash) and
prepend/ensure "set -o pipefail" (or "set -e -o pipefail") before the oc get csr
| awk ... | xargs ... command in the ansible.builtin.shell invocation so
pipeline failures are not silently ignored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ansible/roles/deploy-crc-cloud/tasks/accept_cert.yamlansible/roles/deploy-crc-cloud/tasks/main.yaml
danpawlik
force-pushed
the
retry-command
branch
from
3050619 to
4a5b8cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml (1)
3-7:⚠️ Potential issue | 🟠 Major
pipefailis not guaranteed under the default shell, so retries can silently do nothing.At Line 4,
set -o pipefailmay fail under/bin/sh(common default), and with Line 11ignore_errors: true, all loop iterations can be swallowed without effective CSR approval.🔧 Proposed fix
-- name: Re-run accept certificates in Pending state few times - ansible.builtin.shell: > - set -o pipefail; + ansible.builtin.shell: | + set -o pipefail oc get csr --no-headers | awk '/Pending/ {print $1}' | xargs --no-run-if-empty oc adm certificate approve + args: + executable: /bin/bash loop: "{{ range(1, 4) | list }}" loop_control: pause: 10Also applies to: 11-11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml` around lines 3 - 7, The shell task using "set -o pipefail" can silently no-op under /bin/sh and combined with ignore_errors: true will swallow failures; update the ansible.builtin.shell task in accept_cert.yaml to run under bash by adding executable: /bin/bash (or change the command to a bash -c wrapper) so set -o pipefail is honored, and remove or tighten the ignore_errors setting (or add a proper failure check after the oc adm certificate approve pipeline) to ensure failed approvals are detected and cause the task to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml`:
- Around line 23-25: The retry exit condition is currently checking for any
non-1 return code which will stop retries on unexpected failures; change the
until condition that references the registered result (_cert_approver) from
"until: _cert_approver.rc != 1" to a success check "until: _cert_approver.rc ==
0" so the task keeps retrying until the command actually succeeds; update the
until line that pairs with "register: _cert_approver" (and leave retries: 60
as-is unless you want to adjust the retry count).
---
Duplicate comments:
In `@ansible/roles/deploy-crc-cloud/tasks/accept_cert.yaml`:
- Around line 3-7: The shell task using "set -o pipefail" can silently no-op
under /bin/sh and combined with ignore_errors: true will swallow failures;
update the ansible.builtin.shell task in accept_cert.yaml to run under bash by
adding executable: /bin/bash (or change the command to a bash -c wrapper) so set
-o pipefail is honored, and remove or tighten the ignore_errors setting (or add
a proper failure check after the oc adm certificate approve pipeline) to ensure
failed approvals are detected and cause the task to fail.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ansible/roles/deploy-crc-cloud/tasks/accept_cert.yamlansible/roles/deploy-crc-cloud/tasks/main.yaml
It happens, that the command for accepting cert require to be executed few times and until cron job is not created, something needs to accept the cert. Also move condition to main task, because the tasks should be only executed when variable 'ca_cert_approver' is true. Signed-off-by: Daniel Pawlik <dpawlik@redhat.com>
danpawlik
force-pushed
the
retry-command
branch
from
4a5b8cf to
e013eee
Compare
rdoproject
pushed a commit
to rdo-infra/review.rdoproject.org-config
that referenced
this pull request
Feb 26, 2026
It happens, that the command for accepting cert require to be executed few times and until cron job is not created, something needs to accept the cert. Also move condition to main task, because the tasks should be only executed when variable 'ca_cert_approver' is true. Same patch was applied in crc-cloud project [1] [1] crc-org/crc-cloud#217 Change-Id: Ie3126002f655703d5863ea770ba84e0ba92b2acf Signed-off-by: Daniel Pawlik <dpawlik@redhat.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
2 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.
It happens, that the command for accepting cert require to be executed
few times and until cron job is not created, something needs to accept
the cert.
Also move condition to main task, because the tasks should be only
executed when variable 'ca_cert_approver' is true.
Summary by CodeRabbit