Skip to content
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

sql: Use prefix as name of db to connect to in testing helper #4605

Closed
wants to merge 1 commit into from

Conversation

dt
Copy link
Member

@dt dt commented Feb 23, 2016

Rather than always relying on prefixing table names with db names.
Still may need to do the connect-create-reconnect dance in a few places to actually use this though.

Review on Reviewable

Rather than always relying on prefixing table names with db names
@tamird
Copy link
Contributor

tamird commented Feb 24, 2016

Fixes #4612?

@knz
Copy link
Contributor

knz commented Feb 24, 2016

I think that if you try to merge this right now you will run into #4550. Fix coming in #4610.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


sql/pgwire_test.go, line 293 [r1] (raw file):
I don't think this change is related to this PR.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Feb 24, 2016

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


sql/pgwire_test.go, line 293 [r1] (raw file):
I think it is; pg_url is being changed to include a dbname in the URL, which is reflected in SHOW DATABASE (note that this is the singular SHOW DATABASE, not SHOW DATABASES)


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Feb 24, 2016

Looks like CI passed. LGTM


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: all files reviewed at latest revision, 2 unresolved discussions.


testutils/sqlutils/pg_url.go, line 68 [r1] (raw file):
Why not leave it to the caller to do this? Just wondering why we want this as the default because the caller then needs to know create a database named prefix.


Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Feb 24, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


sql/pgwire_test.go, line 293 [r1] (raw file):
exactly -- before the current db was empty, but after this change, the session default db name is the name passed to the test helper, which this test ran into here.


testutils/sqlutils/pg_url.go, line 68 [r1] (raw file):
Caller could change the URL.Path it gets back -- it just seemed to make sense to have something in there by default. I don't think that callers need to create the DB unless they want to use it -- tests that qualify all their table names should be unchanged as far as I know.


Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: all files reviewed at latest revision, 2 unresolved discussions.


testutils/sqlutils/pg_url.go, line 68 [r1] (raw file):
Yeah, I'm not worried about existing tests which qualify their table names, just that this is only half the work for tests which want to not qualify their table names. Wouldn't it be better for those tests to contain all of the logic about setting the database instead of having it split between this function and the test?


Comments from the review on Reviewable.io

@dt dt closed this Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants