Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a --wait flag feature to enable synchronous waiting for Dagster jobs to complete, supporting CI processes that need to wait for job completion before proceeding.
- Adds a new
waitinput parameter to action configurations with appropriate descriptions - Implements conditional logic to pass the
--waitflag to the underlying Dagster CLI command - Provides proper parameter forwarding between action layers
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
actions/utils/run/action.yml |
Adds the wait input parameter definition with default value and description |
actions/launch_job/action.yml |
Adds the wait input parameter and forwards it to the utils/run action |
src/run.sh |
Implements the conditional logic to append --wait flag based on the input parameter |
src/run.sh
Outdated
| --tags "${INPUT_TAGS_JSON}" \ | ||
| --config-json "${INPUT_CONFIG_JSON}" | ||
| --config-json "${INPUT_CONFIG_JSON}" \ | ||
| $(if [ "$(echo "${INPUT_WAIT}" | tr '[:upper:]' '[:lower:]')" = "true" ]; then echo "--wait"; fi) |
There was a problem hiding this comment.
The condition logic is overly complex with nested quotes and command substitution. Consider simplifying to: $([ "${INPUT_WAIT,,}" = "true" ] && echo "--wait") or use a case statement for better readability.
| $(if [ "$(echo "${INPUT_WAIT}" | tr '[:upper:]' '[:lower:]')" = "true" ]; then echo "--wait"; fi) | |
| $([ "${INPUT_WAIT,,}" = "true" ] && echo "--wait") |
|
Your pull request is automatically being deployed to Dagster Cloud.
|
- Add wait input to launch_job and utils/run actions - Modify run.sh to conditionally include --wait flag - Add debug output to troubleshoot wait functionality - Support various boolean input formats (true, 1, yes, on)
This reverts commit 59375e8.
This reverts commit 8947606.
This reverts commit 674e513.
- Add debug output to troubleshoot command execution - Handle different output formats from dagster-cloud CLI when --wait is used - Extract run ID from status messages like 'Run <id> is in progress' - Fix GITHUB_OUTPUT format issue
- Test run script without wait flag (default behavior) - Test run script with wait flag enabled (multi-line output) - Test various truthy values for wait parameter - Test backwards compatibility when INPUT_WAIT is not set - Maintain original test for backwards compatibility - Handle extraction of run ID from both single line and multi-line outputs
- Make run ID regex more flexible to handle both UUIDs and test strings - Change from strict 36-char UUID pattern to more permissive alphanumeric pattern - Allows tests to pass with mock data like 'some-run' while still working with real UUIDs
|
Tested with both the wait: true and wait removed (existing behavior) and added tests. Also tested on https://github.com/dagster-io/hooli-data-eng-pipelines/pull/186/files to make sure it worked as expected on both wait: true and no wait specified. |
src/run.sh
Outdated
| --repository "${INPUT_REPOSITORY_NAME}" \ | ||
| --job "${INPUT_JOB_NAME}" \ | ||
| --tags "${INPUT_TAGS_JSON}" \ | ||
| --config-json "${INPUT_CONFIG_JSON}" | ||
| --config-json "${INPUT_CONFIG_JSON}" \ | ||
| ${wait_flag} 2>&1 |
There was a problem hiding this comment.
does this stream out the output to the github action logs while the command is running? Just thinking of runs that take a while, would be nice to get that live feedback
There was a problem hiding this comment.
@gibsondan great suggestion! I added the interval and the streaming the log output -- can't test it since I can't release a new version now -- do you want me to request access to test end-to-end before this would get released?
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| # Add interval flag if set | ||
| if [ -n "${interval_flag}" ]; then | ||
| cmd_args+=(${interval_flag}) # This will split --interval and the value |
There was a problem hiding this comment.
There's a potential issue with how the interval flag is being added to the command arguments array. The unquoted ${interval_flag} will undergo word splitting by the shell, which could cause problems if the interval value contains spaces or special characters.
Consider either:
-
Keeping it quoted to preserve it as a single element:
cmd_args+=("${interval_flag}") -
Or if the intention is to split
--intervaland its value into separate array elements, explicitly handle that:if [ -n "${INPUT_INTERVAL}" ]; then cmd_args+=("--interval" "${INPUT_INTERVAL}") fi
The second approach is likely more robust for command-line argument handling.
| cmd_args+=(${interval_flag}) # This will split --interval and the value | |
| if [ -n "${INPUT_INTERVAL}" ]; then | |
| cmd_args+=("--interval" "${INPUT_INTERVAL}") | |
| fi |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
closing in favor of #235 |
2 participants
Adds support to
dagster-cloud-actions/utils/runfor the --wait flag (which was the only parameter not supported by the GitHub action that was in the dagster-cloud job launch. This way, if the action is used as part of a CI workload, the CI can wait until that Dagster job is complete (common for example with dbt slim CI setups)Test plan
--waitflag not present or false on the github action) and multi-line (when--waitis present).add-wait-paramtag for thedagster-cloud-actionpackage in GitHub and tested in hooli to validate that it works on a real invocation with bothwait: trueand wait not specified (the default).I didn't add support for
--intervalbut I have uncommitted code that adds it, I just didn't do so here because it's untested. Happy to push that up for completeness