fix: incorrect default timeout for full run#627
Conversation
WalkthroughUpdates the CI workflow to accept Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/continuous-integration.yml (1)
373-376:matrix.network.timeoutdoesn't exist – gonna hit a snag here, mate!Line 374's reaching for a field that simply isn't in the matrix. The network entries only define
default-timeout-light-runanddefault-timeout-full-run, but this line's trying to grabmatrix.network.timeoutwhich is like trying to find the missing Ark of the Covenant – it's just not there. This'll either result in the timeout being undefined or GitHub Actions falling back to the default, which is basically a blind shot in the dark.Headscratcher too – lines 218-219 get it right, using the actual timeout fields in the environment variables. So this looks like a copy-paste fumble or an incomplete refactor that snuck through.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Test coverage
- GitHub Check: Test the CLI
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
.github/workflows/continuous-integration.yml (3)
31-32: Nice one, mate!Stashing the workflow inputs into top-level env vars is a solid move – keeps the shell scripts clean and avoids the gnarly
${{ github.event.inputs.x }}syntax everywhere. Like organising your inventory in a proper RPG, everything's where you need it.
200-207: Matrix timeout config looks ripper!Adding the light and full run timeouts per network gives you flexibility for tuning – like adjusting your loadout before a boss fight. Both networks sharing the same values for now is grand; easy to tweak later if one needs more headroom.
218-219: Good propagation of defaults to env vars.Mapping the matrix values to env vars makes them accessible in the bash scripts – clean and straightforward.
jeluard
force-pushed
the
jeluard/fix-ci
branch
from
c607a80 to
745bdcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/continuous-integration.yml:
- Around line 200-201: The timeout inputs default-timeout-light-run and
default-timeout-full-run are defined as quoted strings ('15', '60'); change them
to unquoted numeric values (15, 60) so GitHub Actions receives numbers for
timeout-minutes and to silence the actionlint warning; apply the same change for
the other occurrences mentioned (the entries at the same keys on lines 206-207).
- Line 374: The workflow step uses timeout-minutes: ${{ env.TIMEOUT_RUN }} but
TIMEOUT_RUN is never exported to $GITHUB_ENV earlier, so export it when you
compute it by appending a line like echo "TIMEOUT_RUN=<value>" >> $GITHUB_ENV
(or use the workflow env: output and set-env pattern) in the step that
determines TIMEOUT_RUN so the environment variable is actually available to
later steps; ensure the exported value is a plain integer string so
timeout-minutes receives a valid number.
- Around line 244-260: TIMEOUT_RUN is computed in the conditional branches but
never exported to the workflow environment, so downstream steps reading
env.TIMEOUT_RUN will be empty; update the script that sets TIMEOUT_RUN to export
it to $GITHUB_ENV after the value is chosen (e.g., append a line writing
"TIMEOUT_RUN=${TIMEOUT_RUN}" to $GITHUB_ENV) so the TIMEOUT_RUN variable is
available to later steps and to the workflow input used at timeout-minutes;
ensure this export occurs after all branches that set TIMEOUT_RUN (the block
referencing DEMO_TARGET_EPOCH and TIMEOUT_RUN) so the final determined value is
persisted.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/continuous-integration.yml
374-374: type of expression at "float number value" must be number but found type string
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Benches
- GitHub Check: Build riscv32
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/macos
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
- GitHub Check: clippy
🔇 Additional comments (2)
.github/workflows/continuous-integration.yml (2)
31-32: Nice work capturing the workflow inputs, mate!Setting up
INPUT_TIMEOUTandINPUT_DEMO_TARGET_EPOCHas top-level env vars makes them accessible throughout the workflow. Clean approach!
218-219: Solid move propagating matrix values to env vars!Making
DEFAULT_TIMEOUT_LIGHTandDEFAULT_TIMEOUT_FULLavailable as environment variables allows your shell logic to reference them cleanly. This is the way.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
jeluard
force-pushed
the
jeluard/fix-ci
branch
from
745bdcb to
8c40a46
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/continuous-integration.yml:
- Around line 244-260: The workflow_dispatch branch sets DEMO_TARGET_EPOCH but
can leave TIMEOUT_RUN empty (it’s initialized from INPUT_TIMEOUT), so ensure
TIMEOUT_RUN always has a fallback: after the if/elif/else block check if
TIMEOUT_RUN is empty and, if so, set it to DEFAULT_TIMEOUT_LIGHT when
DEMO_TARGET_EPOCH is set (or to DEFAULT_TIMEOUT_FULL for pushes), or simply set
TIMEOUT_RUN="${INPUT_TIMEOUT:-$DEFAULT_TIMEOUT_FULL}"/use DEFAULT_TIMEOUT_LIGHT
when matrix branch selected; update references to INPUT_TIMEOUT, TIMEOUT_RUN,
DEMO_TARGET_EPOCH, DEFAULT_TIMEOUT_LIGHT and DEFAULT_TIMEOUT_FULL to enforce a
non-empty TIMEOUT_RUN.
🧹 Nitpick comments (1)
.github/workflows/continuous-integration.yml (1)
376-376: actionlint's havin' a sook about type coercion here.The
env.TIMEOUT_RUNcomes through as a string, andtimeout-minuteswants a proper number. GitHub Actions should handle the coercion automatically, but usingfromJSON()would make actionlint (and future maintainers) sleep easier at night.🔢 Proposed fix: explicit type conversion
- name: Run node - timeout-minutes: ${{ env.TIMEOUT_RUN }} + timeout-minutes: ${{ fromJSON(env.TIMEOUT_RUN) }} shell: bash run: make AMARU_MAX_EXTRA_LEDGER_SNAPSHOTS=all demo
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/continuous-integration.yml
376-376: type of expression at "float number value" must be number but found type string
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Test coverage
- GitHub Check: Benches
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
.github/workflows/continuous-integration.yml (3)
31-32: Ripper! Clean input capture pattern.Stashing those
workflow_dispatchinputs as env vars makes the downstream shell logic a breeze to read. No dramas here, mate.
200-201: Beauty! You nailed the numeric timeout fix.Swapping those quoted strings for proper numbers – that's the good oil right there. Past feedback addressed like a champ. Both preprod and preview are sorted now.
Also applies to: 206-207
218-219: Solid move exposing those matrix defaults.Making
DEFAULT_TIMEOUT_LIGHTandDEFAULT_TIMEOUT_FULLavailable as env vars keeps your bash script clean as a whistle. Nice one!
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
jeluard
force-pushed
the
jeluard/fix-ci
branch
from
8c40a46 to
a5f0e92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/continuous-integration.yml:
- Line 376: The workflow uses timeout-minutes: ${{ env.TIMEOUT_RUN }} where
env.TIMEOUT_RUN is a string; convert it to a number using fromJSON to satisfy
actionlint and ensure type correctness. Update the expression to parse the
environment variable into a number (e.g., use fromJSON on env.TIMEOUT_RUN) so
timeout-minutes receives a numeric value, keeping the rest of the job unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/continuous-integration.yml
376-376: type of expression at "float number value" must be number but found type string
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build wasm32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/macos
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
.github/workflows/continuous-integration.yml (4)
31-32: LGTM! Clean way to wire up the workflow inputs.Capturing the workflow_dispatch inputs as environment variables with the
INPUT_prefix makes them easy to reference in your bash script later on. Proper setup here, mate!
200-201: Ripper work on the numeric timeouts!The timeout values are now proper numbers (15, 60) instead of strings. That's the way to do it for matrix entries.
Also applies to: 206-207
218-219: Nice one! Pulling the timeout defaults from the matrix.This gives you per-network timeout configuration, which is exactly what you need. The matrix values flow through cleanly to your env vars.
244-264: Solid logic for the timeout dance!The conditional flow handles all the scenarios beautifully:
- PRs get the light timeout (15 min) for quick feedback
- Pushes get the full timeout (60 min) for comprehensive runs
- Manual dispatches can override either the epoch or timeout
- Everything's properly exported to
GITHUB_ENVfor the next actsThe logic tree is sound and covers your bases, cobber!
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/continuous-integration.yml (1)
14-16: The timeout input works a treat, but could use a wee bit more context, mate.The input definition is functionally correct (string type is the only option for numeric inputs in workflow_dispatch), but the description doesn't mention that the value should be in minutes. Adding "(in minutes)" to the description would save future you from a moment of confusion when triggering a manual run.
📝 Proposed documentation enhancement
timeout: - description: "Override timeout for snapshot tests" + description: "Override timeout for snapshot tests (in minutes)" required: false
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build aarch64/macos
- GitHub Check: Test coverage
- GitHub Check: Test the CLI
- GitHub Check: Examples (ledger-in-nodejs)
- GitHub Check: Benches
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
.github/workflows/continuous-integration.yml (5)
31-32: Nice! Setting up the input plumbing like a pro.These environment variables correctly capture the workflow_dispatch inputs and will gracefully default to empty strings for PR and push events. The downstream bash checks handle the empty cases properly, so this is spot on, mate!
200-201: G'day! The timeout values are looking crispy clean now.These are properly defined as numbers (no quotes), which means GitHub Actions will receive proper numeric types for timeout-minutes. The previous linter warning has been squashed like a bug in a Bethesda game! The duplication across networks is totally fine—gives you flexibility to tune per-network later if needed.
Also applies to: 206-207
218-219: Pulling those defaults from the matrix—smooth as butter on a hot potato!The job-level env vars correctly extract the timeout values from the matrix, making them available throughout the job. The matrix provides the numeric values, and GitHub Actions will interpolate them as strings, which is exactly what bash expects downstream.
244-264: The timeout computation logic is tighter than a drum, my friend!This implementation handles all the scenarios like a champion:
- PRs get the quick light timeout (15 min) for speedy feedback
- Pushes to main get the full timeout (60 min) for thorough validation
- Manual workflow dispatch with a specific epoch gets light timeout (treating it like a targeted test)
- And you can always override with the explicit timeout input
The initialization on line 244 ensures TIMEOUT_RUN is never empty—that previous edge case has been handled beautifully! The logic flow is clean and intentional.
Minor note: The timeout input accepts any string value (even "0" or "banana"), but since this is an internal workflow for developers who know what they're doing, that's totally acceptable. No validation needed for this use case, cobber!
376-376: fromJSON() coming in clutch like a last-minute save in FIFA!The conversion from string to number is spot on. Since TIMEOUT_RUN comes through bash and GITHUB_ENV as a string, fromJSON properly converts it to the numeric type that timeout-minutes expects. This silences the actionlint warning and makes the type contract crystal clear—exactly what the previous review requested!
2 participants
Introduce a custom matrix variable for full run tests:
default-timeout-full-run. It is now used for full test runs unless overridden viaworkflow_dispatch.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.