Fix env.sh: portable CLUSTER_DIR, regex, and quoting issues#557
Fix env.sh: portable CLUSTER_DIR, regex, and quoting issues#557rustyrazorblade merged 2 commits intomainfrom
Conversation
|
Good set of targeted fixes. Here is my review. |
|
Good set of targeted fixes. Here is my review: Regex fix is the most impactful change. The original db[0-9*] was genuinely broken: inside [...], * is a literal asterisk, so it matched db followed by one digit OR a literal *. The fixed ^db[0-9]+ (anchored, one-or-more digits) is correct and matches what the error message already described. Portable CLUSTER_DIR - The cd and pwd pattern is the standard POSIX-portable alternative to realpath and correctly handles the source cluster/env.sh use case. ZSH path detection - The ZSH percent-x idiom is the right ZSH way to get the path of the currently executing or sourced script. Better than dollar-zero, which returns the shell name when sourced. Quoting - quoting the ssh host variable and the rsync remote host variable is correct. Quoting the rsync remote path as a single argument is exactly right. Two pre-existing issues worth a follow-up (not introduced by this PR, not blocking):
All four changes are correct. The regex fix closes a real functional bug. No concerns with the implementation. |
Code Coverage
|
|
@claude rebase |
|
Claude finished @rustyrazorblade's task in 19s —— View job I'm unable to perform a rebase due to the limitations of my git capabilities — I can only create and push commits, not perform branch operations like rebase, merge, or reset. To rebase this branch onto main, you can run: git fetch origin
git rebase origin/main
git push origin HEADYou can find more information and potential workarounds in the FAQ. |
- Replace realpath with portable cd+pwd approach for CLUSTER_DIR detection
- Bash: uses BASH_SOURCE[0] with cd && pwd (no realpath dependency)
- ZSH: uses ${(%):-%x} prompt expansion (correct for sourced scripts, not $0)
- Fixes "source cluster/env.sh" from outside the cluster directory
- Fix incorrect regex in flame functions: db[0-9*] to ^db[0-9]+
- Previously matched literal asterisk inside bracket expression
- Now correctly matches one or more digits with anchored prefix
- Quote variables in ssh/rsync calls to handle hostnames with spaces
Fixes #488
Co-authored-by: Jon Haddad <rustyrazorblade@users.noreply.github.com>
9ae8af9 to
d6f438e
Compare
easy-db-lab now builds automatically on every invocation instead of requiring EASY_DB_LAB_REBUILD=1. Two new env vars allow skipping: - EASY_DB_LAB_SKIP_BUILD=1 skips the gradle build - EASY_DB_LAB_SKIP_DOCKER=1 skips docker compose for OTel collector end-to-end-test exports both after its initial build so subsequent easy-db-lab calls don't rebuild or restart containers.
|
PR Review - env.sh portability fixes. The regex, quoting, and CLUSTER_DIR changes are all correct. Three concerns: (1) EASY_DB_LAB_REBUILD=1 is now silently ignored - the flag semantics inverted to EASY_DB_LAB_SKIP_BUILD=1, which is a breaking change worth documenting. (2) EASY_DB_LAB_SKIP_DOCKER conflicts with the project never-disable principle in CLAUDE.md - the end-to-end tests would skip the full docker compose startup path. (3) All five flame functions are missing return 1 after the empty-HOST error message, so ssh runs with an empty host - worth fixing here. Pre-existing: c-dl has a hardcoded cassandra path /mnt/db1/cassandra/artifacts/ which conflicts with the general-purpose db tool principle. |
Fix all shell scripting issues identified in env.sh:
realpathwith portablecd && pwdapproach for CLUSTER_DIR detection (works withsource cluster/env.sh)${(%):-%x}prompt expansion instead of$0db[0-9*]to^db[0-9]+Fixes #488
Generated with Claude Code