Add a wait option for jobs dispatched #301
Conversation
WalkthroughThe changes introduce new configuration options to control waiting for workflow completion, retry intervals, and failure propagation in a GitHub Action. The implementation adds input parameters, extends configuration interfaces, introduces a polling function to monitor workflow completion, and updates tests to cover these new behaviors. Conditional logic in the main workflow now optionally waits for completion. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Action (main.ts)
participant Config (action.ts)
participant API (api.ts)
participant GitHub API
User->>GitHub Action (main.ts): Trigger Action
GitHub Action (main.ts)->>Config (action.ts): getConfig()
Config (action.ts)-->>GitHub Action (main.ts): ActionConfig
GitHub Action (main.ts)->>GitHub API: Dispatch Workflow
GitHub API-->>GitHub Action (main.ts): runId, url
alt waitForRunCompleted = true
GitHub Action (main.ts)->>API (api.ts): waitForDispatch(runId, url)
loop up to maxCompletedFetchAttempts
API (api.ts)->>GitHub API: Get workflow run status
GitHub API-->>API (api.ts): status, conclusion
alt status == "completed"
alt conclusion == "success"
API (api.ts)-->>GitHub Action (main.ts): Success
else conclusion == "failure" or "cancelled"
alt propagateFailures = true
API (api.ts)-->>GitHub Action (main.ts): Throw error
else
API (api.ts)-->>GitHub Action (main.ts): Return
end
end
else
API (api.ts)->>API (api.ts): Sleep interval
end
end
else
GitHub Action (main.ts)->>GitHub Action (main.ts): handleActionSuccess()
end
GitHub Action (main.ts)-->>User: Done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/action.ts (1)
7-9: Remove redundant type annotations.TypeScript can infer these types from the literal values, making the explicit type annotations unnecessary.
-const WORKFLOW_JOB_COMPLETE_RETRY_SECONDS: number = 10; -const WORKFLOW_JOB_COMPLETE_CHECK_MAX_ATTEMPTS: number = 30; // 5 minutes with default 10 seconds between attempts -const WORKFLOW_JOB_COMPLETE_CHECK_PROPAGATE_FAILURES: boolean = false; +const WORKFLOW_JOB_COMPLETE_RETRY_SECONDS = 10; +const WORKFLOW_JOB_COMPLETE_CHECK_MAX_ATTEMPTS = 30; // 5 minutes with default 10 seconds between attempts +const WORKFLOW_JOB_COMPLETE_CHECK_PROPAGATE_FAILURES = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.mjsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
action.yml(1 hunks)src/action.spec.ts(2 hunks)src/action.ts(3 hunks)src/api.spec.ts(3 hunks)src/api.ts(1 hunks)src/main.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/api.ts (1)
src/utils.ts (1)
sleep(64-66)
🪛 ESLint
src/action.ts
[error] 7-7: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 8-8: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 9-9: Type boolean trivially inferred from a boolean literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
🔇 Additional comments (23)
action.yml (2)
32-32: LGTM! Consistent string defaults for GitHub Actions.The change to string defaults for
workflow_timeout_secondsandworkflow_job_steps_retry_secondsis appropriate as GitHub Actions inputs are always strings and need to be parsed.Also applies to: 37-37
40-51: LGTM! Well-designed configuration inputs for wait functionality.The new inputs provide comprehensive control over the wait behavior with sensible defaults:
wait_for_run_completed: Enables the featuremax_completed_fetch_attempts: Reasonable 30 attempts defaultmax_completed_fetch_interval: 10-second intervals balance responsiveness vs API rate limitspropagate_failures: Disabled by default for backwards compatibilitysrc/action.spec.ts (1)
33-36: LGTM! Appropriate test mock extensions.The test modifications correctly extend the mock infrastructure to support the new configuration inputs without changing the core test logic. This maintains test coverage for the expanded configuration parsing.
Also applies to: 60-67
src/main.ts (1)
50-59: LGTM! Clean conditional integration of wait functionality.The implementation properly branches the execution flow based on the
waitForRunCompletedconfiguration. When enabled, it calls the newwaitForDispatchfunction; otherwise, it maintains the original behavior. The logic is clear and preserves backwards compatibility.src/action.ts (3)
60-77: LGTM! Well-structured interface extensions.The new properties are properly documented and typed, providing clear configuration options for the wait functionality.
105-115: LGTM! Consistent configuration parsing.The new configuration parsing follows the established pattern and provides appropriate fallbacks to the constants.
119-132: LGTM! Robust boolean parsing utility.The
getBoolFromValuefunction provides proper validation and error handling for boolean string inputs, with clear error messages for invalid values.src/api.ts (4)
301-310: LGTM! Good user experience with monitoring URL.The function starts by logging the monitoring URL and provides periodic reminders every 30 attempts (5 minutes), which helps users track progress externally.
312-345: LGTM! Comprehensive polling logic with proper state handling.The polling implementation correctly:
- Fetches workflow run status using the GitHub API
- Handles all completion conclusions (success, failure, cancelled, timed_out, etc.)
- Respects the
propagateFailuresconfiguration for error propagation- Implements configurable sleep intervals between attempts
- Provides informative logging for each state
347-356: LGTM! Appropriate timeout handling.The timeout logic properly handles the case when the workflow doesn't complete within the maximum attempts, with behavior controlled by the
propagateFailuresflag.
357-367: LGTM! Robust error handling.The error handling catches both Error instances and unexpected error types, with appropriate logging and conditional re-throwing based on the
propagateFailuresconfiguration.src/api.spec.ts (12)
22-22: LGTM!The import of
waitForDispatchis correctly added to support the new test suite.
30-36: Well-structured mock setup for testing.The mock preserves the actual implementation while only mocking the
sleepfunction to prevent delays during tests. This is a good practice for unit testing asynchronous operations.
361-364: LGTM!The configuration is appropriately extended with the new properties needed for testing the wait functionality.
1059-1078: Well-configured test setup.The test configuration uses appropriate values for testing:
- Smaller retry attempts (3) and intervals (1s) for faster test execution
- Enables
waitForRunCompletedwhich is necessary for testing this functionality- Maintains consistency with other test configurations
1080-1099: Comprehensive success case test.The test correctly validates the happy path scenario with proper mocking of the GitHub API response and appropriate assertions.
1101-1120: LGTM!This test correctly verifies that failures are handled gracefully when
propagateFailuresis false, which is an important behavior to validate.
1122-1143: Good test coverage for failure propagation.The test correctly validates that errors are thrown when
propagateFailuresis true and the workflow fails, with an appropriate error message.
1145-1166: LGTM!This test covers the cancelled workflow scenario well, ensuring that cancellation is treated as a failure when
propagateFailuresis enabled.
1168-1184: Excellent timeout behavior validation.This test correctly verifies the retry logic by ensuring the API is called exactly
maxCompletedFetchAttemptstimes (3) when the workflow remains in a non-terminal state.
1186-1207: Good timeout error propagation test.The test correctly validates that timeout scenarios throw appropriate errors when
propagateFailuresis enabled, with a clear error message indicating the number of attempts.
1209-1218: LGTM!This test ensures that API errors are handled gracefully when
propagateFailuresis false, which is important for robust error handling.
1220-1235: Comprehensive API error propagation test.The test correctly validates that API errors are properly propagated when
propagateFailuresis true, ensuring consistent error handling behavior.
|
Thanks for the submission. Awaiting the completion of a job was intentionally left split into a second action I author, see: https://github.com/Codex-/await-remote-run This is not currently obvious if you only encounter this action in isolation, though, so I will update the readme Is there anything here that is not covered by the usage of await-remote-run? |
|
@randy-coburn-zeam please see the above, awaiting your response before proceeding |
|
@Codex- I had a question on the issue that I raised to match this PR to. https://github.com/Codex-/return-dispatch/issues/302 I just wanted to know why these 2 things are split over 2 actions. I would like to know if I am missing something as in my head I don't understand why it would need 2 actions? I suspect that you can just close and reject this PR since you seem to have made a choice to split these jobs into 2 actions. |
https://github.com/convictional/trigger-workflow-and-wait
This repo has a similar function but fails to correctly identify the jobs that have been created by the dispatch request. However what it does do is wait for the job to be finished and possibly propagate the outcome of the job.
I have added this feature in.
Summary by CodeRabbit
New Features
Bug Fixes
Tests