-
Notifications
You must be signed in to change notification settings - Fork 5
Enable to Switch a DB in ScalarDB Cluster test #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@yito88 Is it ready for review? |
|
@feeblefakie Ah, sorry, I forgot to change the status to open. It's ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the ScalarDB Cluster test configuration to improve flexibility and maintainability. Instead of using a special cluster value for the --db parameter, the PR introduces a cleaner separation where --db specifies the actual database type (e.g., postgres, cassandra) and cluster mode is enabled via the Leiningen profile with-profile cluster. Docker credentials are now passed via environment variables rather than command-line arguments.
Key Changes
- Refactored
--dbparameter to specify the database type rather than the cluster/non-cluster mode - Cluster mode is now controlled by the
cluster?environment variable set by the Leiningen profile - Docker credentials moved from command-line arguments to environment variables (
DOCKER_USERNAME,DOCKER_ACCESS_TOKEN)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scalardb/src/scalardb/runner.clj | Removed "cluster" from supported DBs map, removed docker credential CLI options, updated module loading to check cluster? env var, added db-type parameter to gen-db calls |
| scalardb/src/scalardb/db_extend.clj | Removed unused get-db-type method from DbExtension protocol |
| scalardb/src/scalardb/db/postgres.clj | Updated gen-db signature to accept optional db-type parameter using variadic args |
| scalardb/src/scalardb/db/cluster.clj | Refactored to accept db-type parameter throughout, moved docker credentials to environment variables, improved error handling in wipe! function, added db-type to ExtCluster record |
| scalardb/src/scalardb/db/cassandra.clj | Updated gen-db signature to accept optional db-type parameter using variadic args |
| scalardb/project.clj | Added cluster? environment variable to cluster profile configuration |
| README.md | Updated example command to use environment variables for docker credentials instead of CLI arguments, changed --db cluster to --db postgres |
| .github/workflows/test.yml | Updated workflow to set docker credentials as environment variables and use --db postgres instead of --db cluster |
| .github/workflows/daily.yml | Updated workflow to set docker credentials as environment variables and use --db postgres instead of --db cluster |
| .github/workflows/cluster-test.yml | Updated workflow to use --db postgres instead of --db cluster |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the ScalarDB Cluster test to allow switching the backend database, which is a great enhancement for test flexibility. The changes are logical, moving Docker credentials to environment variables and using a profile to enable cluster testing. My review includes suggestions to improve maintainability by reducing code duplication and to enhance robustness by adding checks for necessary environment variables.
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
To switch DB type for ScalarDB Cluster test
We will be able to easily add other DBs for the ScalarDB Cluster test.
Related issues and/or PRs
Changes made
--dbspecifies the DB instead ofclusterwith-profile clusterChecklist
Additional notes (optional)