Skip to content

[hotfix] Add help handling to build script - #995

Open
kaiwangleo wants to merge 3 commits into
apache:mainfrom
kaiwangleo:hotfix/build-script-help
Open

[hotfix] Add help handling to build script#995
kaiwangleo wants to merge 3 commits into
apache:mainfrom
kaiwangleo:hotfix/build-script-help

Conversation

@kaiwangleo

@kaiwangleo kaiwangleo commented Aug 11, 2026

Copy link
Copy Markdown

Linked issue: N/A (hotfix)

Purpose of change

Add -h/--help handling to tools/build.sh and show usage for unknown options instead of failing because show_help is undefined.

Tests

  • Added tools/test/integration/build_help.bats covering --help, -h, and unknown options.
  • Manually verified bash tools/build.sh --help, bash tools/build.sh -h, and bash tools/build.sh --no-such-option (Bats is not installed locally).

API

No public API changes.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Add -h and --help handling to tools/build.sh and print usage for unknown options. Add Bats regression coverage for the supported help paths and invalid arguments.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Aug 11, 2026
Retrigger the CI matrix after an unrelated flaky TokenMetricsE2ETest failure in the Flink 2.1 job.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)

@weiqingy weiqingy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for taking this on. One question inline, nothing blocking.

Comment thread tools/test/integration/build_help.bats Outdated
@test "build rejects an unknown option with usage" {
run bash "$BUILD_SCRIPT" --no-such-option

[ "$status" -ne 0 ]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: -ne 0 passes whether the script is broken or fixed. On main this path exits 127, and with your fix it exits 1, so both satisfy the check. I tried changing the *) branch to exit 2 locally and all three tests still passed, so nothing here locks in the exit code your fix actually produces. The test does still go red against main, just through the Usage: check below it, so it is doing its job.

Would [ "$status" -eq 1 ] be worth using instead? That would say an unknown option is a usage error rather than a crash. Curious whether you left it loose on purpose.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated in commit 3605821: the unknown-option regression test now asserts the expected usage-error exit code with [ $status -eq 1 ]. The focused Bats test passes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I left the assertion broad initially because the test only needed to verify that an unknown option failed, regardless of the specific non-zero status. However, since tools/build.sh treats unknown options as usage errors and explicitly exits with status 1, asserting [ $status -eq 1 ] makes the expected behavior clearer and prevents unrelated failures such as exit code 127 from satisfying the test. I’ve updated the test in commit 3605821f; the focused Bats tests pass.

Assert that unknown build options return the documented usage-error exit code.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-not-needed Your PR changes do not impact docs labels Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants