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

testserver: use listening URL file to get SQL URL #30

Merged
merged 1 commit into from
May 3, 2017

Conversation

benesch
Copy link
Contributor

@benesch benesch commented May 3, 2017

Use the new --listening-url-file flag to specify a file in which the SQL URL will be written. This flag was added in cockroachdb/cockroach#15468 for exactly this purpose, and makes this testserver resilient to changes in what log output is printed where. Closes #29.

/cc @knz this solves the problem rather neatly :). Really we should never have been parsing stdout.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented May 3, 2017

:lgtm:

Forgot I just merged that 🤦‍♀️


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


testserver/testserver.go, line 76 at r1 (raw file):

	// Import postgres driver.

why the newline?


testserver/testserver.go, line 252 at r1 (raw file):

		ts.mu.Lock()
		if ts.state != stateRunning {
			return fmt.Errorf("server stopped or crashed before listening URL file was available")

this returns without unlocking, which doesn't seem great?


Comments from Reviewable

Use the new `--listening-url-file` flag to specify a file in which the
SQL URL will be written. This flag was added in
cockroachdb/cockroach#15468 for exactly this purpose, and makes this
testserver resilient to changes in what log output is printed where.
Closes #29.
@benesch benesch force-pushed the listening-url-file branch from c983bb1 to defcee4 Compare May 3, 2017 17:06
@benesch
Copy link
Contributor Author

benesch commented May 3, 2017

Yeah, I thought it was pretty hilarious you sheparded that PR.


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


testserver/testserver.go, line 76 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why the newline?

Oops. Thanks.


testserver/testserver.go, line 252 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this returns without unlocking, which doesn't seem great?

Eek! Thanks, fixed.


Comments from Reviewable

@benesch benesch merged commit d4d9299 into master May 3, 2017
@benesch benesch deleted the listening-url-file branch May 3, 2017 17:10
@tamird
Copy link
Contributor

tamird commented May 3, 2017

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


testserver/testserver.go, line 76 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Oops. Thanks.

I think you left it =/


Comments from Reviewable

@benesch
Copy link
Contributor Author

benesch commented May 3, 2017

Eep, so I did. See #33.

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.

2 participants