Skip to content

Conversation

sjpotter
Copy link
Contributor

Attempts an initial connection to the rkt api service via the golang net api. Only try grpc if that succeeds.

@k8s-bot
Copy link
Collaborator

k8s-bot commented Apr 15, 2016

Jenkins GCE e2e

Build/test passed for commit 7f4e9f6.

@@ -37,6 +38,13 @@ var (

func Client() (rktapi.PublicAPIClient, error) {
once.Do(func() {
_, err := net.Dial("tcp", "localhost:15441")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use defaultRktAPIServiceAddr here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use DialTimeout with a reasonable value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connection should be closed

@timstclair
Copy link
Contributor

Thanks for fixing this!

@k8s-bot
Copy link
Collaborator

k8s-bot commented Apr 15, 2016

Jenkins GCE e2e

Build/test passed for commit 3aae756.

@@ -38,17 +38,19 @@ var (

func Client() (rktapi.PublicAPIClient, error) {
once.Do(func() {
_, err := net.Dial("tcp", "localhost:15441")
conn, err := net.DialTimeout("tcp", defaultRktAPIServiceAddr, 2*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you chose a different timeout from the one below? If not, can you create a timeout const to use in both places instead.

@k8s-bot
Copy link
Collaborator

k8s-bot commented Apr 15, 2016

Jenkins GCE e2e

Build/test failed for commit 755073c.

@timstclair
Copy link
Contributor

Looks like you need to run gofmt, then LGTM.

@k8s-bot
Copy link
Collaborator

k8s-bot commented Apr 15, 2016

Jenkins GCE e2e

Build/test passed for commit ed56506.

@timstclair timstclair merged commit 5dc9fba into google:master Apr 15, 2016
@timstclair timstclair self-assigned this Apr 15, 2016
@sjpotter sjpotter deleted the fix-rkt-client branch April 17, 2016 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants