fix(migrations): skip non-directory entries under keyspaces/ - #192
Conversation
|
@sureshkrishnan-ai thanks for the PR. This mentions fixing #190 which actually contains two separate defects. Would you mind splitting that into two issues so that we can track correctly? |
migrations/cassandra/keyspaces/ holds README.md alongside the four keyspace directories. execute_sqls.sh iterated over every entry, so README.md was treated as a keyspace and passed to migrate as x-migrations-table=README.md. The golang-migrate Cassandra driver reads the dot in that value as keyspace.table, so it tried to open a keyspace named README, which Cassandra lowercases: error: failed to open database: Keyspace 'readme' doesn't exist Migration failed for /tmp/keyspaces/README.md execute_sqls.sh then exits 1, which fails the Helm hook job and leaves the cassandra release in a failed state. Guard both loops with [ -d ] so only directories are treated as keyspaces. This also covers any other non-directory file added to keyspaces/ later. Fixes NVIDIA#235 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Suresh Krishnan <sureshkrishnan.ai@gmail.com>
78bf7f8 to
468751c
Compare
📝 WalkthroughWalkthroughThe Cassandra migration script now skips non-directory entries while preprocessing keyspaces and executing migrations, preventing files such as ChangesCassandra migration filtering
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Thanks @nvjmcnamee. Done: split #190 into two.
No code change in the PR itself, just the amended |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/cassandra/execute_sqls.sh`:
- Around line 57-60: The directory-filter guards in the migration script lack
regression coverage. Add a test that places a non-directory file such as
README.md alongside keyspace directories and verifies both preprocessing and
migration process only directories; if testing is not feasible, document the
omission in the merge-request description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e166431-c715-4ddf-a81e-5c5b1e3c67da
📒 Files selected for processing (1)
migrations/cassandra/execute_sqls.sh
|
🎉 This PR is included in version nvcf-cassandra-migrations-v0.13.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
TL;DR
migrations/cassandra/keyspaces/holdsREADME.mdalongside the four keyspace directories, andexecute_sqls.shiterated over every entry, soREADME.mdwas treated as a keyspace and aborted the migration run. This guards both loops with[ -d ]so only directories are treated as keyspaces.Additional Details
README.mdwas passed tomigrateasx-migrations-table=README.md. The golang-migrate Cassandra driver reads the dot in that value askeyspace.table, so it tried to open a keyspace namedREADME, which Cassandra lowercases:execute_sqls.shthen exits 1, which fails thecassandra-helm-nvcf-cassandra-migrationsHelm hook job and leaves thecassandrarelease in afailedstate.The first loop also creates an empty
/tmp/keyspaces/README.mddirectory on the way through, which is why the guard is applied to both loops rather than only the one that callsmigrate. The[ -d ]form was chosen over a.dockerignorefor*.mdbecause it also covers any other non-directory file added tokeyspaces/later.The four real keyspaces (
api_keys_api,ess_api,nvcf_api,sis_api) are unaffected.For the Reviewer
Single file, six lines, no behaviour change for directories.
This fixes the
keyspaces/README.mddefect, now tracked as #235 (split out of #190 at maintainer request). The base-image defect (#190, theFROM bitnami/cassandra:5.0.6base no longer resolving, since Bitnami withdrew their public catalog in Aug 2025) is deliberately not addressed here, because the right replacement looks like a maintainer decision rather than something to presume:bitnamilegacy/cassandra:5.0.5-debian-12-r7builds and runs, but it is a version downgrade, and an internal mirror may be preferred. Happy to follow up with a PR once you indicate a direction.For QA
Verified against the fixed image built with
README.mdpresent inkeyspaces/:End to end, against Cassandra 5.0.x running in a local k3d cluster: with
README.mdpresent in the build context the migrations hook job fails as quoted above; with the same image built from a context whereREADME.mdis removed, the job completes and the schema applies. This change makes theREADME.md-present case behave like theREADME.md-absent case.bash -n migrations/cassandra/execute_sqls.shis clean.QA needed: no, beyond a normal migrations run.
Issues:
Checklist
On tests: there is no existing test harness for the shell scripts under
migrations/(no BUILD target, no bats or shell test suite, and no migrations entry intools/ci/subproject-validations.yaml), so there was no natural place to add one for a six-line guard. Happy to add coverage if you want a harness introduced, or to fold it into whatever you would prefer here.Summary by CodeRabbit