[WIP] MPIIT: Use ExitTrap--PostProcessPrep for CNV (Virt) test mapping#78731
[WIP] MPIIT: Use ExitTrap--PostProcessPrep for CNV (Virt) test mapping#78731oharan2 wants to merge 5 commits intoopenshift:mainfrom
Conversation
WalkthroughThis PR centralizes JUnit post-processing and tightens shell error handling: adds a REPORTPORTAL_CMP env var, replaces inline yq/XML mapping with an EXIT-trap-driven ExitTrap--PostProcessPrep helper, and switches scripts to stricter shell flags and revised runtime variable handling. ChangesTest result mapping & EXIT-trap postprocessing
Sequence DiagramsequenceDiagram
participant Script as Test Script
participant Trap as EXIT Trap
participant Helper as ExitTrap--PostProcessPrep
participant Results as JUnit Results
Script->>Script: Start (set -euo pipefail, inherit_errexit)
Script->>Trap: Install EXIT trap (conditional on MAP_TESTS)
Script->>Script: Run tests (make deploy_test / pytest)
Script->>Results: Produce JUnit XML files
Note over Script,Results: Script exits (success or failure)
Trap->>Helper: Download & invoke ExitTrap--PostProcessPrep (if MAP_TESTS)
Helper->>Results: Rewrite/merge suites using REPORTPORTAL_CMP--%s
Trap->>Script: Run debug_on_exit and exit with original code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 6 minutes and 19 seconds. Comment |
openshift-ci
Bot
added
the
approved
|
@oharan2, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-commands.sh`:
- Around line 62-70: The CNV_WAIT_FOR_LIVE_DEBUG check treats "0" as enabled
because it only tests for non-empty; change the conditional so only a positive
integer enables the debug-sleep path: replace the `if [[ -n
"${CNV_WAIT_FOR_LIVE_DEBUG:-}" ]]; then` check with a numeric test that returns
true only when CNV_WAIT_FOR_LIVE_DEBUG > 0 (e.g., using [[
"${CNV_WAIT_FOR_LIVE_DEBUG:-0}" -gt 0 ]] or (( CNV_WAIT_FOR_LIVE_DEBUG > 0 ))),
and adopt an explicit sentinel value (e.g., "inf") if you need an indefinite
sleep branch so that unset or "0" is treated as disabled in the
CNV_WAIT_FOR_LIVE_DEBUG handling code and in the subsequent conditional that
currently decides between sleep N and sleep inf.
- Around line 7-15: The startup block currently evals a remote script
immediately (when MAP_TESTS="true") which can fail under set -e and prevents
running make deploy_test; instead, remove the early eval and change the trap to
fetch-and-source the helper at EXIT time with robust error handling: in the trap
callback (referencing ExitTrap--PostProcessPrep, REPORTPORTAL_CMP and
junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml) curl the raw URL pinned to
an immutable commit hash to a temp file, check curl's exit status and only
source the file if the download succeeded (log a warning but do not abort if it
failed), then call ExitTrap--PostProcessPrep and
junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml processing; ensure no
commands in the startup path use eval of remote content so make deploy_test
always runs even if the fetch fails.
In
`@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh`:
- Around line 261-268: The post-processing block can override the real pytest
exit code because set -e is enabled and a failing curl/eval will change the
script exit status; change the block around MAP_TESTS so the curl+eval for
ExitTrap--PostProcessPrep is best-effort: temporarily disable errexit (e.g., set
+e) before running curl -fsSL ... and eval, or append || true to the curl/eval
pipeline, then re-enable errexit (set -e) and restore/exit with the original
pytest rc variable; also replace the refs/heads/main URL with a pinned commit
SHA in the curl URL to ensure reproducibility. Ensure you reference MAP_TESTS,
ExitTrap--PostProcessPrep, LP_IO__ET_PPP__NEW_TS_NAME and
junit--cnv__interop-tests__openshift-virtualization-tests.xml when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9cc36788-dda5-450e-9dbc-f1822371edda
📒 Files selected for processing (6)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__cnv-odf-ocp-4.22-lp-interop.yamlci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-commands.shci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-ref.yamlci-operator/step-registry/interop-tests/ocs-tests/interop-tests-ocs-tests-ref.yamlci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.shci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-ref.yaml
| if [ "${MAP_TESTS}" = "true" ]; then | ||
| eval "$( | ||
| curl -fsSL \ | ||
| https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh | ||
| )"; trap ' | ||
| LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \ | ||
| ExitTrap--PostProcessPrep junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml | ||
| ' EXIT | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-commands.shRepository: openshift/release
Length of output: 3352
Don't let the helper fetch gate make deploy_test.
Lines 7-15 fetch and eval a script from refs/heads/main before the test run at line 58. With set -e enabled, any GitHub/raw outage or curl failure will fail this step before make deploy_test executes, even though this block only provides post-processing. Additionally, the mutable ref (refs/heads/main) makes the step non-reproducible. Defer this fetch to exit time with proper error handling and pin to an immutable commit hash instead.
Suggested direction
-if [ "${MAP_TESTS}" = "true" ]; then
- eval "$(
- curl -fsSL \
-https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh
- )"; trap '
- LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \
- ExitTrap--PostProcessPrep junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml
- ' EXIT
-fi
+post_process_junit() {
+ [ "${MAP_TESTS}" = "true" ] || return 0
+
+ if eval "$(
+ curl -fsSL \
+https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/<pinned-commit>/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh
+ )"; then
+ LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \
+ ExitTrap--PostProcessPrep junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml || \
+ echo "Warning: junit post-processing failed; leaving results unmodified" >&2
+ else
+ echo "Warning: unable to load ExitTrap--PostProcessPrep; leaving results unmodified" >&2
+ fi
+}
+
+trap post_process_junit EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-commands.sh`
around lines 7 - 15, The startup block currently evals a remote script
immediately (when MAP_TESTS="true") which can fail under set -e and prevents
running make deploy_test; instead, remove the early eval and change the trap to
fetch-and-source the helper at EXIT time with robust error handling: in the trap
callback (referencing ExitTrap--PostProcessPrep, REPORTPORTAL_CMP and
junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml) curl the raw URL pinned to
an immutable commit hash to a temp file, check curl's exit status and only
source the file if the download succeeded (log a warning but do not abort if it
failed), then call ExitTrap--PostProcessPrep and
junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml processing; ensure no
commands in the startup path use eval of remote content so make deploy_test
always runs even if the fetch fails.
There was a problem hiding this comment.
@coderabbitai
This has been discussed over and over again. You are supposed to learn this. See my comment and in any other PR about the the test nature is non-deterministic to begin with.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh api repos/openshift/release/pulls/comments/3183387185 --jq '.body'
Length of output: 404
@etirta, thank you for the clarification and apologies for the repeated noise. I now understand: ExitTrap--PostProcessPrep is mandatory for Component Readiness (CR) to pick up results — if the network fetch fails, failing the entire step is the correct and intentional behavior, because silently showing a "successful" test in CR without the post-processing would be misleading and harmful. The early eval+curl placement is a deliberate design decision.
I've added a learning to avoid raising this concern in future reviews.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| # Map results by setting identifier prefix in tests suites names for reporting tools | ||
| # Merge original results into a single file and compress | ||
| # Send modified file to shared dir for Data Router Reporter step (run here so EXIT stays debug_on_exit). | ||
| if [ "${MAP_TESTS}" = "true" ]; then |
There was a problem hiding this comment.
@etirta The issue in this script is that the trap command is occupied with the debug_on_exit, witch PQE said they will not give up on.
Instead of wrapping a new trap function that calls the following (you said it's a bad practice):
function newTrapWrapper ():
...
ExitTrap--PostProcessPrep
debug_on_exit
...
, I thought about using it as regular command (not trap).
- From working on this scenario, I understand that the XMLs won't be generated before the previous command (uv), and since the exit code is being piped and not fail the script on failure, our mapping will execute in any way.
Thoughts?
There was a problem hiding this comment.
@oharan2 Merge them.
if [ "${MAP_TESTS}" = "true" ]; then
eval "$(
curl -fsSL \
https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh
)"; trap '
debug_on_exit
LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \
ExitTrap--PostProcessPrep junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml
' EXIT
else
trap 'debug_on_exit' EXIT
fiThere is no need to create a new function newTrapWrapper just to call those 2 functions, we can call those directly instead.
There was a problem hiding this comment.
Thanks. I will apply it in all relevant scenarios, such as odf (for example).
In this specific case, debug_on_exit is exisiting the script with the exit code, so I have changed the order. See my last modifications.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oharan2 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 |
|
@oharan2, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
oharan2
changed the title
oharan2
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh`:
- Around line 12-16: The EXIT trap currently invokes debug_on_exit first which
calls exit and prevents the following ExitTrap--PostProcessPrep commands from
running; to fix, reorder the trap body so ExitTrap--PostProcessPrep (the block
that sets LP_IO__ET_PPP__NEW_TS_NAME using REPORTPORTAL_CMP and calls
junit--cnv__interop-tests__openshift-virtualization-tests.xml) runs before
calling debug_on_exit, or alternatively modify debug_on_exit to avoid calling
exit when invoked from the trap; update the trap invocation that references
debug_on_exit and ExitTrap--PostProcessPrep so post-processing executes first
and debug_on_exit runs last (preserving exit code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4da39e36-5b47-46b7-92d4-c800362e93fc
📒 Files selected for processing (2)
ci-operator/step-registry/interop-tests/cnv-tests-e2e-deploy/interop-tests-cnv-tests-e2e-deploy-ref.yamlci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh (1)
5-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
ExitTrap--PostProcessPrepbootstrap non-fatal and always install an EXIT trap.With
set -eenabled, a transient curl/eval failure in theMAP_TESTS=truepath (line 9-12) terminates the script before the trap is installed. This skips test execution and final debug handling. Wrap the helper load in an if/else to ensure an EXIT trap is always set before continuing.Suggested change
if [ "${MAP_TESTS}" = "true" ]; then # Map results by setting identifier prefix in tests suites names for reporting tools # Merge original results into a single file and compress # Send modified file to shared dir for Data Router Reporter step (run here so EXIT stays debug_on_exit). - eval "$( - curl -fsSL \ -https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh - )"; trap ' - typeset scriptExit=$? - LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \ - ExitTrap--PostProcessPrep junit--cnv__interop-tests__openshift-virtualization-tests.xml || true - debug_on_exit "${scriptExit}" - ' EXIT + if eval "$( + curl -fsSL \ +https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh + )"; then + trap ' + typeset scriptExit=$? + LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \ + ExitTrap--PostProcessPrep junit--cnv__interop-tests__openshift-virtualization-tests.xml || true + debug_on_exit "${scriptExit}" + ' EXIT + else + echo "WARN: could not load ExitTrap--PostProcessPrep; continuing without MAP_TESTS post-processing" >&2 + trap 'debug_on_exit' EXIT + fi else trap 'debug_on_exit' EXIT fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh` around lines 5 - 20, The current eval of the ExitTrap--PostProcessPrep helpers (the curl|eval block that defines ExitTrap--PostProcessPrep) can fail under set -e and prevent any EXIT trap from being installed; make the bootstrap non-fatal and ensure an EXIT trap is always registered: first install a minimal fallback trap (e.g., trap 'debug_on_exit' EXIT) unconditionally, then attempt to load the helper with curl|eval inside a conditional that handles failures (so the eval failure does not exit the script), and only if the helper loads successfully replace the fallback trap with the richer trap that calls ExitTrap--PostProcessPrep and junit--cnv__interop-tests__openshift-virtualization-tests.xml; reference the MAP_TESTS check, the curl/eval block that pulls ExitTrap--PostProcessPrep, and the trap that currently contains ExitTrap--PostProcessPrep to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh`:
- Line 236: The curl pipeline that downloads and extracts oc using ocUrl and
binFolder is not resilient to transient network issues; update the download step
to use curl with fail-fast and retry/timeouts (e.g., add --fail, --retry with a
small count, --retry-connrefused, and connection/total timeouts like
--connect-timeout/--max-time) and ensure the command exits non-zero on failure
before piping to tar (so tar doesn't hang on an empty stream). Apply these
options to the existing usage of ocUrl and preserve extraction into binFolder
(targeting the same oc binary).
---
Duplicate comments:
In
`@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh`:
- Around line 5-20: The current eval of the ExitTrap--PostProcessPrep helpers
(the curl|eval block that defines ExitTrap--PostProcessPrep) can fail under set
-e and prevent any EXIT trap from being installed; make the bootstrap non-fatal
and ensure an EXIT trap is always registered: first install a minimal fallback
trap (e.g., trap 'debug_on_exit' EXIT) unconditionally, then attempt to load the
helper with curl|eval inside a conditional that handles failures (so the eval
failure does not exit the script), and only if the helper loads successfully
replace the fallback trap with the richer trap that calls
ExitTrap--PostProcessPrep and
junit--cnv__interop-tests__openshift-virtualization-tests.xml; reference the
MAP_TESTS check, the curl/eval block that pulls ExitTrap--PostProcessPrep, and
the trap that currently contains ExitTrap--PostProcessPrep to locate where to
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 42f63b8a-25bc-48e5-aef1-c06336218e70
📒 Files selected for processing (1)
ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh
| ########################################################################### | ||
| # Get oc binary | ||
| curl -sL "${OC_URL}" | tar -C "${BIN_FOLDER}" -xzvf - oc | ||
| curl -sL "${ocUrl}" | tar -C "${binFolder}" -xzvf - oc |
There was a problem hiding this comment.
Harden oc download against transient network failures.
curl -sL here has no fail-fast, retry, or timeout controls. A flaky network can make the step fail intermittently or hang longer than needed.
Suggested change
-curl -sL "${ocUrl}" | tar -C "${binFolder}" -xzvf - oc
+curl --fail --silent --show-error --location \
+ --retry 5 --retry-all-errors --connect-timeout 30 --max-time 300 \
+ "${ocUrl}" | tar -C "${binFolder}" -xzvf - oc📝 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.
| curl -sL "${ocUrl}" | tar -C "${binFolder}" -xzvf - oc | |
| curl --fail --silent --show-error --location \ | |
| --retry 5 --retry-all-errors --connect-timeout 30 --max-time 300 \ | |
| "${ocUrl}" | tar -C "${binFolder}" -xzvf - oc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/interop-tests/openshift-virtualization-tests/interop-tests-openshift-virtualization-tests-commands.sh`
at line 236, The curl pipeline that downloads and extracts oc using ocUrl and
binFolder is not resilient to transient network issues; update the download step
to use curl with fail-fast and retry/timeouts (e.g., add --fail, --retry with a
small count, --retry-connrefused, and connection/total timeouts like
--connect-timeout/--max-time) and ensure the command exits non-zero on failure
before piping to tar (so tar doesn't hang on an empty stream). Apply these
options to the existing usage of ocUrl and preserve extraction into binFolder
(targeting the same oc binary).
|
@oharan2: The following test 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. |
etirta
left a comment
etirta
left a comment
There was a problem hiding this comment.
When introducing changes, please maintain consistency, as it leads to higher quality. I noticed that some curl commands were updated to follow the -fsSL Best Practices, but not all of them.
Nit: While shell scripts are very lax regarding variable types, it is generally better to be explicit by using typeset -i for variables meant to hold integer values.
| if [ "${MAP_TESTS}" = "true" ]; then | ||
| eval "$( | ||
| curl -fsSL \ | ||
| https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh | ||
| )"; trap ' | ||
| LP_IO__ET_PPP__NEW_TS_NAME="${REPORTPORTAL_CMP}--%s" \ | ||
| ExitTrap--PostProcessPrep junit--cnv__interop-tests__cnv-tests-e2e-deploy.xml | ||
| ' EXIT | ||
| fi |
There was a problem hiding this comment.
@coderabbitai
This has been discussed over and over again. You are supposed to learn this. See my comment and in any other PR about the the test nature is non-deterministic to begin with.
| curl -fsSL \ | ||
| https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/ci-operator/interop/common/ExitTrap--PostProcessPrep.sh | ||
| )"; trap ' | ||
| typeset scriptExit=$? |
There was a problem hiding this comment.
scriptExit is a somewhat peculiar name for storing the actual exit status. Terms like es, ec, or rc (corresponding to exit status, exit code, and return code, respectively) are more commonly used.
| local exit_code=$? | ||
| # First argument is the script exit status when the EXIT trap ran other handlers first (e.g. ExitTrap--PostProcessPrep). | ||
| local exit_code="${1:-$?}" | ||
| local end_time=$SECONDS | ||
| local execution_time=$((end_time - start_time)) | ||
| local debug_threshold=720 # 12 minutes in seconds | ||
| local hco_namespace=openshift-cnv |
There was a problem hiding this comment.
It is never safe to use $? in this manner. The $? variable is dynamic and holds the exit status of the most recently executed command or shell statement. Depending on how the function is called, there is no guarantee it will hold the exit status of the actual script.
| typeset -i exit_code="${1:?MUST give the actual script Exit Status.}"; (($#)) && shift | |
| typeset -i start_time="${1:?'NEED `start_time` parameter.'}"; (($#)) && shift | |
| typeset -i execution_time=$((SECONDS - start_time)) | |
| typeset -i debug_threshold=720 # 12 minutes in seconds | |
| typeset hco_namespace=openshift-cnv |
It is not a best practice to directly use variables from outside a function's scope. Doing so can lead to subtle, hard-to-find errors and unexpected behavior. Always explicitly pass variable values to the function. For example, in this case, you should provide the start_time from the caller.
3 participants
Main changes:
lp-ocp-compat--CNVSummary by CodeRabbit
New Features
Bug Fixes
Chores