Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6491 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. |
|
/hold until OCPBUGS-73815 is resolved |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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 |
openshift-ci
bot
added
the
approved
|
/test e2e-aws-tests-periodic-arm |
|
/test e2e-aws-tests-periodic |
|
/test e2e-aws-tests-bootc-release e2e-aws-tests-release e2e-aws-tests-bootc-release-arm e2e-aws-tests-release-arm |
|
/test e2e-aws-tests-bootc-release |
6 similar comments
|
/test e2e-aws-tests-bootc-release |
|
/test e2e-aws-tests-bootc-release |
|
/test e2e-aws-tests-bootc-release |
|
/test e2e-aws-tests-bootc-release |
|
/test e2e-aws-tests-bootc-release |
|
/test e2e-aws-tests-bootc-release |
WalkthroughAdds runtime GitOps version discovery to a test utility, expands and renames a GitOps robot test to deploy and verify an application, introduces an Argo CD AppProject/Application for Spring Pet Clinic, and removes an obsolete test Deployment manifest. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@agullon: This pull request references USHIFT-6491 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/suites/gitops/gitops.robot (2)
45-48: Teardown may fail if sync didn't complete.If the application never synced, the namespace might not exist, causing
Oc Delete nsto fail. Consider adding error handling or using--ignore-not-found.♻️ Suggested change
Teardown Application Deployment [Documentation] Teardown the application deployment - Oc Delete -f ${APPLICATION_MANIFEST_PATH} - Oc Delete ns ${APPLICATION_NAMESPACE} + Oc Delete -f ${APPLICATION_MANIFEST_PATH} --ignore-not-found + Oc Delete ns ${APPLICATION_NAMESPACE} --ignore-not-found🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/gitops/gitops.robot` around lines 45 - 48, The teardown step "Teardown Application Deployment" can fail when the application never synced because the namespace may not exist; update the two Oc Delete calls (the one deleting the manifest at ${APPLICATION_MANIFEST_PATH} and the namespace at ${APPLICATION_NAMESPACE}) to tolerate missing resources by either adding the --ignore-not-found flag to the oc delete commands or wrapping the namespace deletion in a Robot keyword that ignores errors (e.g., Run Keyword And Ignore Error or Run Keyword And Continue On Failure) so the teardown proceeds even if the namespace is absent.
26-26: Hardcoded version in documentation URL.The URL references
/4.20/which will become stale. Consider using a version-agnostic link or noting this needs updates with new releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/gitops/gitops.robot` at line 26, The test contains a hardcoded docs URL with "/4.20/" which will become stale; update the string in test/suites/gitops/gitops.robot by replacing the versioned path with a version-agnostic link (remove the "/4.20/" segment) or pull the URL from a variable (e.g., ${MICROSHIFT_DOCS_URL}) so it can be updated centrally, or add a clear TODO comment near the URL indicating it must be reviewed on new releases; target the literal URL in the file when making the change.test/bin/pyutils/generate_common_versions.py (1)
224-224: Catching broadException.Per static analysis (BLE001), catching bare
Exceptioncan mask unexpected errors. Consider catchingrequests.RequestExceptionspecifically.♻️ Suggested change
- except Exception as e: + except requests.RequestException as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` at line 224, Replace the broad "except Exception as e" in the try/except that wraps the HTTP/request logic with a more specific exception class: catch requests.RequestException (i.e., change the handler to "except requests.RequestException as e") and ensure the requests module is imported where generate_common_versions.py performs the network call; this narrows the error handling to request-related failures while leaving other exceptions to surface.test/suites/gitops/spring-petclinic-app.yaml (1)
16-35: Consider reducing external repository dependency in tests.The Application references
https://github.com/siamaksade/openshift-gitops-getting-started, an external third-party repository. While currently accessible, relying on external repos in tests introduces flakiness risk if the repository changes, is deleted, or becomes unavailable. Consider mirroring the required content or using a stable fork maintained within your organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/gitops/spring-petclinic-app.yaml` around lines 16 - 35, The Application manifest currently points to an external third‑party repo (source.repoURL: https://github.com/siamaksade/openshift-gitops-getting-started) which can cause flaky tests; replace that external dependency by either mirroring the required content into a stable internal repo or embedding the test app into the test fixtures and updating the Application to use your mirror/local source. Concretely, update the Application (metadata.name: spring-petclinic, spec.source.path: app, spec.source.repoURL) to point to your organization's mirror or a local test bundle (or change spec.source to reference the bundled fixtures), and ensure any CI manifests or documentation are updated to reference the new repoURL so tests no longer depend on the external GitHub repository.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 291-294: The code calls get_gitops_version(minor_version) and
assigns it to gitops_version but later uses the hardcoded GITOPS_VERSION
constant; update the code to use the dynamically fetched gitops_version where
GITOPS_VERSION is currently referenced (or remove the unused assignment if you
intend to keep the constant), e.g., replace usages of GITOPS_VERSION with
gitops_version so the result of get_gitops_version(minor_version) is actually
applied.
- Around line 220-232: The retry loop incorrectly checks `if attempt == 3` to
detect failure which is always true after the loop; instead detect failure by
using a for-else or an explicit success flag. Update the loop around `for
attempt in range(1, 4):` that calls `requests.get`/`resp.raise_for_status()` so
that on successful fetch you break and on exhausting retries you run the error
handling (e.g., move the `logging.error(f"Failed to fetch data from {url} after
3 attempts")` and `return ""` into the for-else else-branch or add a boolean
`success` set to True on success and test `if not success:`), referencing the
`attempt` variable, `resp`, and the `requests.get` call to ensure failure is
only logged when all attempts truly failed.
---
Nitpick comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Line 224: Replace the broad "except Exception as e" in the try/except that
wraps the HTTP/request logic with a more specific exception class: catch
requests.RequestException (i.e., change the handler to "except
requests.RequestException as e") and ensure the requests module is imported
where generate_common_versions.py performs the network call; this narrows the
error handling to request-related failures while leaving other exceptions to
surface.
In `@test/suites/gitops/gitops.robot`:
- Around line 45-48: The teardown step "Teardown Application Deployment" can
fail when the application never synced because the namespace may not exist;
update the two Oc Delete calls (the one deleting the manifest at
${APPLICATION_MANIFEST_PATH} and the namespace at ${APPLICATION_NAMESPACE}) to
tolerate missing resources by either adding the --ignore-not-found flag to the
oc delete commands or wrapping the namespace deletion in a Robot keyword that
ignores errors (e.g., Run Keyword And Ignore Error or Run Keyword And Continue
On Failure) so the teardown proceeds even if the namespace is absent.
- Line 26: The test contains a hardcoded docs URL with "/4.20/" which will
become stale; update the string in test/suites/gitops/gitops.robot by replacing
the versioned path with a version-agnostic link (remove the "/4.20/" segment) or
pull the URL from a variable (e.g., ${MICROSHIFT_DOCS_URL}) so it can be updated
centrally, or add a clear TODO comment near the URL indicating it must be
reviewed on new releases; target the literal URL in the file when making the
change.
In `@test/suites/gitops/spring-petclinic-app.yaml`:
- Around line 16-35: The Application manifest currently points to an external
third‑party repo (source.repoURL:
https://github.com/siamaksade/openshift-gitops-getting-started) which can cause
flaky tests; replace that external dependency by either mirroring the required
content into a stable internal repo or embedding the test app into the test
fixtures and updating the Application to use your mirror/local source.
Concretely, update the Application (metadata.name: spring-petclinic,
spec.source.path: app, spec.source.repoURL) to point to your organization's
mirror or a local test bundle (or change spec.source to reference the bundled
fixtures), and ensure any CI manifests or documentation are updated to reference
the new repoURL so tests no longer depend on the external GitHub repository.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 98820775-45cf-4747-9234-c39e48948504
📒 Files selected for processing (4)
test/bin/pyutils/generate_common_versions.pytest/suites/gitops/gitops.robottest/suites/gitops/spring-petclinic-app.yamltest/suites/gitops/test-deployment.yaml
💤 Files with no reviewable changes (1)
- test/suites/gitops/test-deployment.yaml
|
@agullon: This pull request references USHIFT-6491 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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
217-229:⚠️ Potential issue | 🟠 MajorFix third-attempt success handling in the retry loop.
Line 227 treats any third-loop iteration as failure. If the first two requests fail and the third succeeds, this still returns
"", which can blank outGITOPS_VERSIONin the generated output.🐛 Proposed fix
for attempt in range(1, 4): try: resp = requests.get(url, params=params, timeout=10) resp.raise_for_status() + break except Exception as e: logging.warning(f"Attempt {attempt} failed with error: {e}. Retrying...") - time.sleep(2) - continue - break - - if attempt == 3: + if attempt < 3: + time.sleep(2) + else: logging.error(f"Failed to fetch data from {url} after 3 attempts") return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 217 - 229, The retry loop incorrectly treats any execution where attempt == 3 as failure even if the third try succeeded; update the logic around the for-loop that uses attempt, requests.get and resp/resp.raise_for_status so that failure is only logged when all attempts actually failed—e.g., replace the post-loop if attempt == 3 check with a proper for-else or a success flag that only triggers logging.error(...) and return "" when no successful resp was obtained (ensure you reference the resp variable after the loop to decide success).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Line 313: The format call in generate_common_versions.py is being passed a key
LATEST_RELEASE_TYPE that is undefined and unused by the template; remove
LATEST_RELEASE_TYPE from the dict/context passed to the .format(...) invocation
(or from the keyword args to format_template) so the undefined name is no longer
referenced. Locate the .format(...) call that currently includes
LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE and delete that key (or replace it with
a defined variable if the template actually needs it), ensuring the rest of the
format context (e.g., any variables like VERSION, CHANNEL, etc.) remains
unchanged.
---
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 217-229: The retry loop incorrectly treats any execution where
attempt == 3 as failure even if the third try succeeded; update the logic around
the for-loop that uses attempt, requests.get and resp/resp.raise_for_status so
that failure is only logged when all attempts actually failed—e.g., replace the
post-loop if attempt == 3 check with a proper for-else or a success flag that
only triggers logging.error(...) and return "" when no successful resp was
obtained (ensure you reference the resp variable after the loop to decide
success).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8b6b5de5-7193-44cb-989b-28889d1c4c2d
📒 Files selected for processing (1)
test/bin/pyutils/generate_common_versions.py
| CNCF_SYSTEMD_LOGS_VERSION=CNCF_SYSTEMD_LOGS_VERSION, | ||
| GITOPS_VERSION=GITOPS_VERSION, | ||
| GITOPS_VERSION=gitops_version, | ||
| LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script=$(fd '^generate_common_versions\.py$' | head -n1)
template=$(fd '^common_versions\.sh\.template$' | head -n1)
rg -n '\bLATEST_RELEASE_TYPE\b' "$script" "$template"Repository: openshift/microshift
Length of output: 161
🏁 Script executed:
script=$(fd '^generate_common_versions\.py$' | head -n1)
cat -n "$script" | sed -n '305,320p'Repository: openshift/microshift
Length of output: 751
🏁 Script executed:
script=$(fd '^generate_common_versions\.py$' | head -n1)
rg -n 'LATEST_RELEASE_TYPE' "$script" -B5 -A5Repository: openshift/microshift
Length of output: 478
🏁 Script executed:
fd '^common_versions\.sh\.template$'Repository: openshift/microshift
Length of output: 103
🏁 Script executed:
script=$(fd '^generate_common_versions\.py$' | head -n1)
template=$(fd '^common_versions\.sh\.template$' | head -n1)
# Check if LATEST_RELEASE_TYPE is defined/imported in script
rg -n 'LATEST_RELEASE_TYPE\s*=' "$script"
# Check template for any usage
cat "$template" | grep -i 'LATEST_RELEASE_TYPE' || echo "NOT FOUND in template"Repository: openshift/microshift
Length of output: 138
🏁 Script executed:
script=$(fd '^generate_common_versions\.py$' | head -n1)
# Show the function this code is in and its imports/definitions
cat -n "$script" | sed -n '280,325p'Repository: openshift/microshift
Length of output: 2238
🏁 Script executed:
script=$(fd '^generate_common_versions\.py$' | head -n1)
# Check for LATEST_RELEASE_TYPE in the entire function scope
cat -n "$script" | sed -n '1,330p' | grep -i 'latest_release_type'Repository: openshift/microshift
Length of output: 119
🏁 Script executed:
# Confirm the template exists and check its first few lines
template=$(fd '^common_versions\.sh\.template$')
echo "=== Template file path: $template ==="
head -20 "$template"Repository: openshift/microshift
Length of output: 705
Remove LATEST_RELEASE_TYPE from the format context on line 314.
LATEST_RELEASE_TYPE is undefined in this scope, and the template doesn't reference this placeholder. Passing an undefined variable to format() causes a NameError at runtime.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 313-313: undefined name 'LATEST_RELEASE_TYPE'
(F821)
🪛 Ruff (0.15.5)
[error] 313-313: Undefined name LATEST_RELEASE_TYPE
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bin/pyutils/generate_common_versions.py` at line 313, The format call in
generate_common_versions.py is being passed a key LATEST_RELEASE_TYPE that is
undefined and unused by the template; remove LATEST_RELEASE_TYPE from the
dict/context passed to the .format(...) invocation (or from the keyword args to
format_template) so the undefined name is no longer referenced. Locate the
.format(...) call that currently includes
LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE and delete that key (or replace it with
a defined variable if the template actually needs it), ensuring the rest of the
format context (e.g., any variables like VERSION, CHANNEL, etc.) remains
unchanged.
|
@agullon: This pull request references USHIFT-6491 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.
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
314-314:⚠️ Potential issue | 🔴 Critical
LATEST_RELEASE_TYPEis undefined — runtimeNameError.This variable doesn't exist in this module. The script will crash when
generate_common_versions()is called.
,🐛 Proposed fix: Remove the undefined parameter
CNCF_SONOBUOY_VERSION=CNCF_SONOBUOY_VERSION, CNCF_SYSTEMD_LOGS_VERSION=CNCF_SYSTEMD_LOGS_VERSION, GITOPS_VERSION=gitops_version, - LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE, ARCH=ARCHIf the template actually needs this value, define it first or pass an appropriate value.
#!/bin/bash # Verify LATEST_RELEASE_TYPE is not defined anywhere and check if template uses it echo "=== Searching for LATEST_RELEASE_TYPE definition in script ===" rg -n 'LATEST_RELEASE_TYPE\s*=' test/bin/pyutils/generate_common_versions.py || echo "Not found in script" echo -e "\n=== Checking if template references this placeholder ===" fd 'common_versions.sh.template' --exec cat {} | grep -i 'LATEST_RELEASE_TYPE' || echo "Not found in template"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` at line 314, The call to generate_common_versions() (or the dict/kwargs passed into it) includes LATEST_RELEASE_TYPE which is not defined and will raise a NameError; remove LATEST_RELEASE_TYPE from the arguments passed (or alternatively define LATEST_RELEASE_TYPE with the correct value before the call) and, if the template common_versions.sh.template actually needs that placeholder, ensure you set a meaningful value (e.g., DEFAULT/rc/stable) into LATEST_RELEASE_TYPE prior to invoking generate_common_versions(); locate the parameter usage around the generate_common_versions / LATEST_RELEASE_TYPE occurrence and either delete the LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE entry or add a proper definition above the call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Line 314: The call to generate_common_versions() (or the dict/kwargs passed
into it) includes LATEST_RELEASE_TYPE which is not defined and will raise a
NameError; remove LATEST_RELEASE_TYPE from the arguments passed (or
alternatively define LATEST_RELEASE_TYPE with the correct value before the call)
and, if the template common_versions.sh.template actually needs that
placeholder, ensure you set a meaningful value (e.g., DEFAULT/rc/stable) into
LATEST_RELEASE_TYPE prior to invoking generate_common_versions(); locate the
parameter usage around the generate_common_versions / LATEST_RELEASE_TYPE
occurrence and either delete the LATEST_RELEASE_TYPE=LATEST_RELEASE_TYPE entry
or add a proper definition above the call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 48308a06-2251-4120-8b43-889ee7c1d2eb
📒 Files selected for processing (1)
test/bin/pyutils/generate_common_versions.py
…ersion and exercise documentation app pre-commit.check-secrets: ENABLED
|
@agullon: This pull request references USHIFT-6491 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/bin/pyutils/generate_common_versions.py (1)
289-313:⚠️ Potential issue | 🟠 MajorFail fast when
gitops_versioncannot be resolved.If
get_gitops_version(minor_version)returns"", this still renderscommon_versions.shwith an emptyGITOPS_VERSION. That turns a transient lookup problem into a broken generated file.Suggested fix
logging.info("Getting GITOPS_VERSION") gitops_version = get_gitops_version(minor_version) + if not gitops_version: + raise RuntimeError("Unable to determine GITOPS_VERSION") template_path = pathlib.Path(__file__).resolve().parent / '../../assets/common_versions.sh.template'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 289 - 313, The script currently allows get_gitops_version(minor_version) to return an empty string and continues to render common_versions.sh with GITOPS_VERSION empty; change the flow after calling get_gitops_version so that if gitops_version is falsy/empty you log an error (including minor_version) and abort (raise/exit) instead of proceeding to template_string.format; update references around get_gitops_version, gitops_version and the template rendering block to perform this validation and fail fast.
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
217-231:⚠️ Potential issue | 🟠 MajorHTTP error responses still bypass the retry failure check.
Line 220 assigns
respbeforeraise_for_status(). If the last attempt returns 4xx/5xx,respis still non-None, so Line 228 is skipped and the code falls through intoresp.json()on a failed response.Suggested fix
resp = None for attempt in range(1, 4): try: - resp = requests.get(url, params=params, timeout=10) - resp.raise_for_status() + candidate = requests.get(url, params=params, timeout=10) + candidate.raise_for_status() + resp = candidate break - except Exception as e: + except requests.RequestException as e: logging.warning(f"Attempt {attempt} failed with error: {e}. Retrying...") time.sleep(2) continue if resp is None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 217 - 231, The bug is that resp is set before calling resp.raise_for_status(), so a final 4xx/5xx response leaves resp non-None and the code calls resp.json() on an error response; fix by deferring assignment until after the status check: inside the retry loop call a temporary variable (e.g., r = requests.get(...)), call r.raise_for_status(), then set resp = r and break; keep the except path to log and continue so resp remains None on failure and the existing post-loop check before resp.json() works correctly.
🧹 Nitpick comments (1)
test/suites/gitops/gitops.robot (1)
45-48: Make teardown idempotent.If setup fails halfway or cleanup reruns, these deletes can fail and mask the real test error.
--ignore-not-found=truekeeps teardown safe.Suggested fix
- Oc Delete -f ${APPLICATION_MANIFEST_PATH} - Oc Delete ns ${APPLICATION_NAMESPACE} + Oc Delete -f ${APPLICATION_MANIFEST_PATH} --ignore-not-found=true + Oc Delete ns ${APPLICATION_NAMESPACE} --ignore-not-found=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/gitops/gitops.robot` around lines 45 - 48, Make the teardown idempotent by adding the --ignore-not-found=true flag to both Oc Delete calls so failed or repeated teardowns don't error: update the Oc Delete that uses ${APPLICATION_MANIFEST_PATH} and the Oc Delete that deletes namespace ${APPLICATION_NAMESPACE} to include --ignore-not-found=true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/gitops/gitops.robot`:
- Around line 29-35: The pod readiness check after Wait Until Resource Exists
and the Oc Wait for application ${APPLICATION_NAME} is racy because Synced can
occur before pods are created; modify the test to first poll for the pod
existence (use oc get pods --selector=app=${APPLICATION_NAME} in
${APPLICATION_NAMESPACE} with a short retry loop and timeout) and only when at
least one pod is returned run the Oc Wait ... pod
--selector=app=${APPLICATION_NAME} --for=condition=Ready --timeout=...; update
the Robot steps around Wait Until Resource Exists / Oc Wait to add this
existence-check retry before the readiness-wait.
---
Outside diff comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 289-313: The script currently allows
get_gitops_version(minor_version) to return an empty string and continues to
render common_versions.sh with GITOPS_VERSION empty; change the flow after
calling get_gitops_version so that if gitops_version is falsy/empty you log an
error (including minor_version) and abort (raise/exit) instead of proceeding to
template_string.format; update references around get_gitops_version,
gitops_version and the template rendering block to perform this validation and
fail fast.
---
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 217-231: The bug is that resp is set before calling
resp.raise_for_status(), so a final 4xx/5xx response leaves resp non-None and
the code calls resp.json() on an error response; fix by deferring assignment
until after the status check: inside the retry loop call a temporary variable
(e.g., r = requests.get(...)), call r.raise_for_status(), then set resp = r and
break; keep the except path to log and continue so resp remains None on failure
and the existing post-loop check before resp.json() works correctly.
---
Nitpick comments:
In `@test/suites/gitops/gitops.robot`:
- Around line 45-48: Make the teardown idempotent by adding the
--ignore-not-found=true flag to both Oc Delete calls so failed or repeated
teardowns don't error: update the Oc Delete that uses
${APPLICATION_MANIFEST_PATH} and the Oc Delete that deletes namespace
${APPLICATION_NAMESPACE} to include --ignore-not-found=true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8320cefc-5249-44ee-a964-f08c53181579
📒 Files selected for processing (4)
test/bin/pyutils/generate_common_versions.pytest/suites/gitops/gitops.robottest/suites/gitops/spring-petclinic-app.yamltest/suites/gitops/test-deployment.yaml
💤 Files with no reviewable changes (1)
- test/suites/gitops/test-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/suites/gitops/spring-petclinic-app.yaml
| Wait Until Resource Exists applications ${APPLICATION_NAME} ${GITOPS_NAMESPACE} timeout=120s | ||
| Oc Wait | ||
| ... -n ${GITOPS_NAMESPACE} application ${APPLICATION_NAME} | ||
| ... --for=jsonpath='{.status.sync.status}'=Synced --timeout=300s | ||
| Oc Wait | ||
| ... -n ${APPLICATION_NAMESPACE} pod --selector=app=${APPLICATION_NAME} | ||
| ... --for=condition=Ready --timeout=300s |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the relevant section
if [ -f "test/suites/gitops/gitops.robot" ]; then
echo "=== File exists, showing lines 20-40 ==="
cat -n test/suites/gitops/gitops.robot | sed -n '20,40p'
else
echo "File not found at expected location"
find . -name "gitops.robot" -type f 2>/dev/null | head -5
fiRepository: openshift/microshift
Length of output: 1200
🏁 Script executed:
# Search for other uses of "Oc Wait" and "Synced" in the codebase to understand patterns
echo "=== Searching for Oc Wait usage patterns ==="
rg "Oc Wait" test/ --context 2 -A 2 -B 2 | head -50
echo -e "\n=== Searching for Application Synced patterns ==="
rg "Synced" test/ --context 1 | head -30Repository: openshift/microshift
Length of output: 4388
🏁 Script executed:
# Look for Wait Until Keyword Succeeds usage in the test suite
echo "=== Checking Wait Until Keyword Succeeds patterns ==="
rg "Wait Until Keyword Succeeds" test/ --context 1 | head -20Repository: openshift/microshift
Length of output: 1566
Pod readiness check is racy after Synced status.
Synced confirms Argo CD applied manifests but does not guarantee pods exist. When Oc Wait executes, the selector can still match zero pods, causing immediate failure instead of polling.
Wrap the pod wait with retry logic to handle the creation delay:
Suggested fix
- Oc Wait
- ... -n ${APPLICATION_NAMESPACE} pod --selector=app=${APPLICATION_NAME}
- ... --for=condition=Ready --timeout=300s
+ Wait Until Keyword Succeeds 5min 10s
+ ... Oc Wait
+ ... -n ${APPLICATION_NAMESPACE} pod --selector=app=${APPLICATION_NAME}
+ ... --for=condition=Ready --timeout=30s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/gitops/gitops.robot` around lines 29 - 35, The pod readiness
check after Wait Until Resource Exists and the Oc Wait for application
${APPLICATION_NAME} is racy because Synced can occur before pods are created;
modify the test to first poll for the pod existence (use oc get pods
--selector=app=${APPLICATION_NAME} in ${APPLICATION_NAMESPACE} with a short
retry loop and timeout) and only when at least one pod is returned run the Oc
Wait ... pod --selector=app=${APPLICATION_NAME} --for=condition=Ready
--timeout=...; update the Robot steps around Wait Until Resource Exists / Oc
Wait to add this existence-check retry before the readiness-wait.
|
/retest |
|
@agullon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit
New Features
Tests
Chores