-
Notifications
You must be signed in to change notification settings - Fork 354
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
TOOLS-2708: Atlas recommended connection string for mongostat doesn't work #314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
opts.Port
andopts.Host
are not set when a user only uses--uri
I think this is the underlying bug though. What you have works, but I think it's more of a band-aid. I think we should fix the problem in ToolOptions.setOptionsFromURI()
instead.
3c20028
to
be5908c
Compare
@@ -596,6 +596,20 @@ func (opts *ToolOptions) setOptionsFromURI(cs connstring.ConnString) error { | |||
return ConflictingArgsErrorFormat("host", strings.Join(cs.Hosts, ","), opts.Host, "--host") | |||
} | |||
} | |||
} else if len(cs.Hosts) > 0 { | |||
hostPort := strings.Split(cs.Hosts[0], ":") | |||
opts.Host = hostPort[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think we have to loop through cs.Hosts
and construct a comma separated list of the hosts without ports for opts.Host
. Also if cs.ReplicaSet
is set, we should prepend <replSet>/
to the list.
It looks like util.CreateConnectionAddrs()
expects a comma separated list of hosts so we need to make sure we set it correctly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't see your question about this on the mtc PR - I'll respond there.
opts.Host = hostPort[0] | ||
|
||
// a port might not be specified, e.g. `mongostat --discover` | ||
if len(hostPort) == 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good check, we should do this for each of the elements of cs.Hosts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm once the mtc PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after revendor
The connection string failure seems to be a result of this code change. Since
opts.Port
andopts.Host
are not set when a user only uses--uri
,fullhost
gets set tolocalhost:27017
, thereby incorrectly rewriting the hostname tolocalhost
.Manual testing:
--discover
without a connection string--uri