Skip to content

Do not give workflow boolean inputs default values - #3014

Merged
zcbenz merged 1 commit into
ml-explore:mainfrom
zcbenz:fix-dry-run
Jan 20, 2026
Merged

Do not give workflow boolean inputs default values#3014
zcbenz merged 1 commit into
ml-explore:mainfrom
zcbenz:fix-dry-run

Conversation

@zcbenz

@zcbenz zcbenz commented Jan 18, 2026

Copy link
Copy Markdown
Member

There is a pitfall in GitHub Actions that the default value of workflow_dispatch inputs can only be string, so when setting default: true we might get a string "true" or a boolean false as default value (it seems that we ran into the latter case).

This PR just removes the default values so inputs would all default to boolean false for pushes.

Also note that I'm using a non-exist env in name: ${{ inputs.dry_run && 'dry-run' || 'pypi' }} because inputs.dry_run && '' || 'pypi' would always evaluate to pypi regardless of the input value, which is another pitfall.

Checked a few cases and they all seem to work as expected:

- uses: ./.github/actions/build-docs

deploy_documentation:
if: ${{ github.event_name == 'push' || inputs.publish }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about removing that: github.event_name == 'push'

I added it because these steps weren't getting triggered because the inputs were not defined when it's a push (vs when it's a manual trigger).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not defined will !inputs.dry_run evaluate to true?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for pushes the !inputs.dry_run would evaluate to true so it defaults to publish run, unlike previous condition inputs.publish which defaults to dry run.

I did a push-triggered run to verify it:
https://github.com/ml-explore/mlx/actions/runs/21111546178
(it ran the deploy_documentation job, unlike the dry run that did not.)

It is still possible that another pitfall would cause the push-triggered publish to fail, but when that happens I think we can just do a manually-triggered publish and fix it later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it thanks for clarifying!

@zcbenz
zcbenz merged commit 65b42c8 into ml-explore:main Jan 20, 2026
15 checks passed
@zcbenz
zcbenz deleted the fix-dry-run branch January 20, 2026 06:27
jzdziarski pushed a commit to jzdziarski/mlx that referenced this pull request Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants