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 |
|
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 runtime GitOps version discovery to a test utility, expands and renames a GitOps robot test to deploy and verify an application, introduces Argo CD AppProject/Application for spring-petclinic, 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)
📝 Coding Plan
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
|
@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
agullon
force-pushed
the
USHIFT-6491
branch
2 times, most recently
from
7afbc84 to
a1e58e0
Compare
|
@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
|
/retest |
1 similar comment
|
/retest |
agullon
force-pushed
the
USHIFT-6491
branch
2 times, most recently
from
f1aae17 to
d13388b
Compare
|
/hold cancel because https://redhat.atlassian.net/browse/OCPBUGS-73815 resolved and new version released |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@agullon: An error was encountered searching for bug USHIFT-6491 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
openshift-ci-robot
removed
the
jira/valid-reference
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
289-292:⚠️ Potential issue | 🟠 MajorAdd validation to prevent empty GITOPS_VERSION in rendered template.
When
get_gitops_version()fails (after 3 attempts or if no compatible version exists), it returns an empty string. This empty value is then passed directly to the template rendering at line 313, producing an invalid config file. Add an explicit check immediately after the call:Proposed fix
logging.info("Getting GITOPS_VERSION") gitops_version = get_gitops_version(minor_version) + if not gitops_version: + raise RuntimeError(f"Unable to resolve GitOps version for OCP 4.{minor_version}")🤖 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 - 292, After calling get_gitops_version(minor_version) and assigning gitops_version, add a validation that gitops_version is non-empty; if it's empty, log an explicit error (using logging.error) that no compatible GITOPS_VERSION was found for the given minor_version and abort the process (raise SystemExit or call sys.exit(1)) before proceeding to template rendering (where gitops_version is used). Ensure you update the block around the gitops_version assignment and the subsequent template rendering path so an empty string can never be passed into the template.
🤖 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 233-237: The loop in generate_common_versions.py reads
gitops_version_ocp_compatibility without guarding its type; to fix, ensure
gitops_version_ocp_compatibility is normalized before the membership check by
replacing the direct use with a safe value (e.g., compat =
gitops_version_from_api_docs.get("openshift_compatibility") or "" then convert
to string or join if it's a list) and then perform the check using
f"4.{current_microshift_minor_version}" in str(compat) (or iterate over compat
if you normalize it to a list); update the conditional that uses
gitops_version_ocp_compatibility to use this normalized variable so it never
raises TypeError.
---
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 289-292: After calling get_gitops_version(minor_version) and
assigning gitops_version, add a validation that gitops_version is non-empty; if
it's empty, log an explicit error (using logging.error) that no compatible
GITOPS_VERSION was found for the given minor_version and abort the process
(raise SystemExit or call sys.exit(1)) before proceeding to template rendering
(where gitops_version is used). Ensure you update the block around the
gitops_version assignment and the subsequent template rendering path so an empty
string can never be passed into the template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5a2d4f09-9788-4d42-a72d-865aeb387f95
📒 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 (2)
- test/suites/gitops/spring-petclinic-app.yaml
- test/suites/gitops/gitops.robot
…ersion and exercise documentation app pre-commit.check-secrets: ENABLED
|
/retest |
|
@agullon: all tests passed! 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. |
| for current_microshift_minor_version in range(minor_version, minor_version - 4, -1): | ||
| for gitops_version_from_api_docs in data.get("data", [{}])[0].get("versions", []): | ||
| gitops_version_ocp_compatibility = gitops_version_from_api_docs.get("openshift_compatibility") or [] | ||
| if isinstance(gitops_version_ocp_compatibility, str): | ||
| gitops_version_ocp_compatibility = [gitops_version_ocp_compatibility] |
There was a problem hiding this comment.
From what I see, the openshift_compatibility is a string, not an array: "openshift_compatibility": "4.20, 4.19, 4.18, 4.17, 4.16, 4.14, 4.21", so the script doesn't work for me in current form.
Otherwise, this is pretty neat idea.
| for current_microshift_minor_version in range(minor_version, minor_version - 4, -1): | |
| for gitops_version_from_api_docs in data.get("data", [{}])[0].get("versions", []): | |
| gitops_version_ocp_compatibility = gitops_version_from_api_docs.get("openshift_compatibility") or [] | |
| if isinstance(gitops_version_ocp_compatibility, str): | |
| gitops_version_ocp_compatibility = [gitops_version_ocp_compatibility] | |
| gitops_version_ocp_compatibility = gitops_version_from_api_docs.get("openshift_compatibility") or "" | |
| gitops_version_number = gitops_version_from_api_docs.get("name") | |
| if f"4.{current_microshift_minor_version}" in gitops_version_ocp_compatibility: |
3 participants
Uh oh!
There was an error while loading. Please reload this page.