Fix PostgreSQL Metadata Source Tests #9
Conversation
|
@isinyaaa can you also take a look? |
There was a problem hiding this comment.
Thanks for the fix, Chanakya!
I have a final nitpick that I'd prefer if you split the two fixes on this PR in separate commits, because they kinda overlap although I think each requires a little more context, e.g. why are you removing quotes on one place and adding them on another? My point being that the changes are similar but for different reasons.
Because we're squash merging I guess you can also "copy-paste" the PR description on a single commit. That's just a click-saver to avoid getting into GitHub to try and figure out why the change was made (although the Mercurial syncs from upstream don't help much on this project).
TLDR: see comments inline, also https://cbea.ms/git-commit is a personal favorite -- especially useful in the open source world.
| absl::Substitute("CREATE DATABASE $0;", config_.dbname().data()); | ||
| absl::Substitute("CREATE DATABASE \"$0\";", config_.dbname().data()); |
There was a problem hiding this comment.
IIUC, and also from the PR description, this fixes DB creation for names containing a "-", as you also raised on a previous meeting.
Can you split that into a separate commit, with the comment from the PR description? I believe it's important to keep track of the reason why certain changes were made.
IIRC we also need that fix on the MySQL metadata source ConnectImpl, can you confirm/test?
.vscode/settings.json
Outdated
| { | ||
| "files.associations": { | ||
| "string": "cpp" | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: you could squash this change to keep the PR cleaner.
Updates Metadata Source Tests after changes from commit
Updates database creation and deletion commands (PSQL specific)
Description
Changes in commit no longer require EscapeString to be surrounded by single quotes.
Previously, databases cannot be created or deleted with the name "model-registry" due to "-". PR surrounds database name in double quotes.
How Has This Been Tested?
Deployed model-registry-operator with a postgres server and ran
standalone_postgresql_metadata_source_test. All 7 tests pass.Merge criteria: