Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Implement Ping for vsphere #6778
Conversation
| + if err != nil { | ||
| + return errors.New("Invalid endpoint format, please give a full url or IP/hostname.") | ||
| + } | ||
| + } else { |
perrito666
Jan 6, 2017
Contributor
if we are going to be forgiving then check also that there is no other protocol passed before just prepending one.
natefinch
Jan 6, 2017
Contributor
Is there a valid url we'll be rejecting or an invalid url we'll be accepting?
| + } | ||
| + // There's no way to get at any type information in the returned error, so | ||
| + // we have to just look at the string value. | ||
| + if err.Error() == "ServerFaultCode: Cannot complete login due to an incorrect user name or password." { |
perrito666
Jan 6, 2017
Contributor
Make that string into a const somewhere, we cant ensure we will not need various strings as vsphere evolves and only one string in an if is coming back to bite us.
|
!!build!! |
jameinel
reviewed
Jan 10, 2017
I had a couple surprises but overall I think LGTM with some tweaks to asserts, etc.
| + // try to be smart and not punish people for adding or forgetting http | ||
| + u, err := url.Parse(endpoint) | ||
| + if err != nil { | ||
| + return errors.New("Invalid endpoint format, please give a full url or IP/hostname.") |
reedobrien
Jan 11, 2017
Contributor
Yes, but it interprets the string as the URL path.
"Parse parses rawurl into a URL structure. The rawurl may be relative or absolute."
natefinch
Jan 13, 2017
Contributor
Yep. which is why I then check the scheme, and if it's empty, then I know that they likely just used a hostname, so I prepend https:// and append /sdk
| + defer logout() | ||
| + err = client.Login(context.TODO(), nil) | ||
| + if err == nil { | ||
| + // shouldn't happen, but can't really complain if it does. |
jameinel
Jan 10, 2017
Owner
It shouldn't happen that we succeed in logging in? Can you spell out something more concrete like "we logged in without credentials successfully, which is surprising but ..."
I wonder if it would actually be a failure as it could be you succeeded an HTTP request to a static page.
Is the login process more of a conversation?
natefinch
Jan 13, 2017
Contributor
The login process is a SOAP conversation. I think it would be quite remarkable if you happened to connect to a machine that responded in the correct way to make us think we're logging into vsphere.
But yes, I can make the explanation more clear.
| + } | ||
| + // There's no way to get at any type information in the returned error, so | ||
| + // we have to just look at the string value. | ||
| + if err.Error() == failedLoginMsg { |
jameinel
Jan 10, 2017
Owner
I'm worried this is very brittle to vSphere version or library version, etc
natefinch
Jan 10, 2017
Contributor
I agree, but there's no type information exposed... the error is an unexported type. Suggestions welcome.
| + // creds, so return nil. | ||
| + return nil | ||
| + } | ||
| + return errors.Errorf("No VSphere server running at %s", endpoint) |
jameinel
Jan 10, 2017
Owner
We should at least be logging the original error that leads us to believe something is wrong for debugging purposes.
natefinch
Jan 10, 2017
Contributor
Yes, that's true, thanks. I forget that logging actually does something on the client.
| +func (pingSuite) TestPingInvalidHost(c *gc.C) { | ||
| + tests := []string{ | ||
| + "foo.com", | ||
| + "http://IHopeNoOneEverBuysThisVerySpecificJujuDomainName.com", |
jameinel
Jan 10, 2017
Owner
.invalid is expressly an invalid tld and I think example.com is also similarly designed for use. But foo.invalid is more what I would use here.
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + err = provider.Ping("abc%sdef") | ||
| + if err == nil { |
natefinch
Jan 13, 2017
Contributor
Yeah, I think I used to have more logic inside the if statement. But you're right, it's extraneous now.
| + } | ||
| + expected := "Invalid endpoint format, please give a full url or IP/hostname." | ||
| + if err.Error() != expected { | ||
| + c.Errorf("expected %q got %v", expected, err) |
| + | ||
| + err = provider.Ping("gopher://abcdef.com") | ||
| + if err == nil { | ||
| + c.Fatal("expected error, but got nil.") |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Unfortunately, I had to remove the feature test that tested interactive add cloud, because it's not possible to test that successfully unless you have a real vsphere environment to connect to. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$wegoagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$findlxdthistimetrusty$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$giveupgiveup$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Generating tarball failed |
natefinch commentedJan 6, 2017
This change updates the stub Ping method to actually attempt
a connection and login to VSphere in order to be more
likely to discover when the user has typoed the endpoint.
QA: