-
Notifications
You must be signed in to change notification settings - Fork 94
Add Discovery Chain Post #378
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 Discovery Chain Post #378
Conversation
marcin-krystianc
left a comment
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.
Hi @larrytamnjong,
I left comments about compile-dc argument, looks good otherwise!
Consul/DiscoveryChain.cs
Outdated
|
|
||
| public class DiscoveryChainOptions | ||
| { | ||
| public string EvaluateInDatacenter { get; set; } |
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.
I think this should not be a part of this structure as it is being used as [compile-dc](https://developer.hashicorp.com/consul/api-docs/discovery-chain#compile-dc) which is a query-only parameter.
Consul/DiscoveryChain.cs
Outdated
| var request = _client.Post<DiscoveryChainOptions, DiscoveryChainResponse>($"/v1/discovery-chain/{name}", options, q); | ||
| if (options != null && options.EvaluateInDatacenter != null) | ||
| { | ||
| request.Params["compile-dc"] = options.EvaluateInDatacenter; |
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.
This should be a separate argument passed to the Get method.
Thanks @marcin-krystianc I have made an update based on your feedback. |
marcin-krystianc
left a comment
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.
Looks good, please just remove those unnecessary empty lines.
Consul.Test/DiscoveryChainTest.cs
Outdated
| Assert.Equal(targetCheck.Name, targetChain.Name); | ||
| } | ||
| } | ||
|
|
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.
Nit: Please remove extra empty lines
Consul.Test/DiscoveryChainTest.cs
Outdated
| } | ||
|
|
||
| } | ||
|
|
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.
Nit: Please remove extra empty lines
Consul.Test/DiscoveryChainTest.cs
Outdated
|
|
||
| } | ||
|
|
||
|
|
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.
Nit: Please remove extra empty lines
Done :) |
marcin-krystianc
left a comment
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.
Nice 👍
#355