-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add regions endpoint #495
Add regions endpoint #495
Conversation
defer s.peerLock.RUnlock() | ||
|
||
regions := make(map[string][]string, len(s.peers)) | ||
for region, servers := range s.peers { |
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.
With this implementation, we will get only the regions where the Nomad servers are present. Users might expect the API to return the entire list of regions where the Nomad cluster spans. We might need to change the name of the API or document it to make sure people understand the purpose of the API.
if err := msgpackrpc.CallWithCodec(codec, "Region.List", &arg, &out); err != nil { | ||
t.Fatalf("err: %v", err) | ||
} | ||
if len(out) != 2 || out[0] != "region1" || out[1] != "region2" { |
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.
Is this going to be flaky? Is there any guarantee on order?
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 call, tests didn't indicate anything but a quick sort.Strings()
should fix this up.
LGTM |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This adds an API call for fetching the list of regions known to Nomad. It also returns a list of the datacenter names nested under each. Related: #243