Adding Discovery Chain GET and POST and tests for it #356
Conversation
Had to comment out OnlyPassing in Configuration.cs, because it causes an error
Consul/Configuration.cs
Outdated
| [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | ||
| public bool OnlyPassing { get; set; } | ||
| //[JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | ||
| //public bool OnlyPassing { get; set; } |
There was a problem hiding this comment.
Is it intentional change?
There was a problem hiding this comment.
@marcin-krystianc
Yep, the class structure is different from Golang code, and with OnlyPassing not commented out it causes an error. I was still kinda testing things out, so I guess I should edit the structure to align with Golang code?
https://github.com/hashicorp/consul/blob/v1.9.17/api/config_entry_discoverychain.go#L136
There was a problem hiding this comment.
yes, please update this struct to align it with the golang code.
There was a problem hiding this comment.
yes, please update this struct to align it with the golang code.
Just did, waiting for the tests to run
There was a problem hiding this comment.
@marcin-krystianc tests just finished, went good
yes, please update this struct to align it with the golang code.
Just did, waiting for the tests to run
Consul/DiscoveryChain.cs
Outdated
|
|
||
| /// <summary> | ||
| /// OverrideMeshGateway allows for the mesh gateway setting to be overridden | ||
| /// For any resolver in the compiled chain. |
Consul/DiscoveryChain.cs
Outdated
|
|
||
| /// <summary> | ||
| /// OverrideConnectTimeout allows for the ConnectTimeout setting to be | ||
| /// Overridden for any resolver in the compiled chain. |
| [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | ||
| public string ConnectTimeout { get; set; } | ||
| public TimeSpan? ConnectTimeout { get; set; } |
There was a problem hiding this comment.
It breaks backward compatibility, but it is ok to do here. Configuration is a fresh addition and I anticipate it is not heavily used yet.
There was a problem hiding this comment.
Okey, sounds good, I was going off of existing code and changed it for consistency
Consul/Configuration.cs
Outdated
| public class ServiceResolverSubset | ||
| { | ||
| public string Filter { get; set; } | ||
| public bool OnlyPassing { get; set; } | ||
| } | ||
|
|
||
| public class ServiceResolverRedirect | ||
| { | ||
| public string Service { get; set; } | ||
| public string ServiceSubset { get; set; } | ||
| public string Namespace { get; set; } | ||
| public string Partition { get; set; } | ||
| public string Datacenter { get; set; } | ||
| public string Peer { get; set; } | ||
| public string SamenessGroup { get; set; } | ||
| } | ||
|
|
||
| public class ServiceResolverFailover | ||
| { | ||
| public string Service { get; set; } | ||
| public string ServiceSubset { get; set; } | ||
| // Referencing other partitions is not supported. | ||
| public string Namespace { get; set; } | ||
| public List<string> Datacenters { get; set; } | ||
| public List<ServiceResolverFailoverTarget> Targets { get; set; } | ||
| public ServiceResolverFailoverPolicy Policy { get; set; } | ||
| public string SamenessGroup { get; set; } | ||
| } | ||
|
|
||
| public class ServiceResolverFailoverTarget | ||
| { | ||
| public string Service { get; set; } | ||
| public string ServiceSubset { get; set; } | ||
| public string Partition { get; set; } | ||
| public string Namespace { get; set; } | ||
| public string Datacenter { get; set; } | ||
| public string Peer { get; set; } | ||
| } | ||
|
|
||
| public class ServiceResolverFailoverPolicy | ||
| { | ||
| public string Mode { get; set; } | ||
| public List<string> Regions { get; set; } |
There was a problem hiding this comment.
Would it be possible to test these structs as well?
In the past we added some new structs without proper tests and they turned out to be incorrectly defined.
There was a problem hiding this comment.
@marcin-krystianc So, I am working on the tests right now, and I also realized, that I have implimented ServiceResolverEntry according with the latests version of Consul API. That shouldn't harm backwards compitability, but that should provide some compatibility with future versions and less work in the future. Should I just leave it at that, or cut out the new fields to avoid confusion? Also, the Golang tests for the ServiceResolver class include some other test - for ServiceSplitter and ServiceRouter, should I write tests for them too, or just leave it at ServiceResolver? https://github.com/hashicorp/consul/blob/v1.9.17/api/config_entry_discoverychain_test.go . I will push the tests I currently have for it in a minute
Would it be possible to test these structs as well? In the past we added some new structs without proper tests and they turned out to be incorrectly defined.
There was a problem hiding this comment.
Should I just leave it at that, or cut out the new fields to avoid confusion?
Please keep the new fields in.
There was a problem hiding this comment.
Should I just leave it at that, or cut out the new fields to avoid confusion?
Please keep the new fields in.
@marcin-krystianc
I think I have gotten the tests sorted up. Could you check the code?
…tructure to be up-to-date with last Consul API version
…hanov/consuldotnet into add-discovery-chain-get
@marcin-krystianc
Added the Discovery Chain Endpoint, tests for it, edits in Client.cs and IConsulClient.cs for adding new class DiscoveryChain.cs. Changes in Configuration for consistency - TimeSpan? ConnectTimeout, changes to ServiceResolverEntry