Skip to content
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

consuldotnet no longer seems to be actively maintained #85

Closed
Choc13 opened this issue Nov 26, 2019 · 5 comments
Closed

consuldotnet no longer seems to be actively maintained #85

Choc13 opened this issue Nov 26, 2019 · 5 comments

Comments

@Choc13
Copy link
Collaborator

Choc13 commented Nov 26, 2019

consuldotnet is now archived and doesn't seem to have been updated recently. Given it is a wrapper around Consul's REST API it should be possible to remove our dependency on it and call the REST API directly. Also, we only need to query a single endpoint in this lib, so the implementation using something like Flurl shouldn't be too difficult.

@davidalpert
Copy link
Contributor

would you be looking to implement this as a breaking change, reworking the current configuration options interface?

or looking to build enough internal classes to mirror the consuldotnet interface such that this would be transparent to Winton.Extensions.Configuration.Consul clients?

for example

  • would you expect this replacement to be achieved by introducing an internal IConsulClient interface and an internal ConsulClient class to wrap the Flurl web calls and poling concerns?
  • would you introduce an internal copy of the ConsulClientConfigurationOptions class to avoid breaking the public ConsulConfigurationSource contract?

@davidalpert
Copy link
Contributor

after a bit of experimentation, I don't see how this can be a non-breaking change, since the consuldotnet ConsulClientConfiguration class is exposed directly on the ConsulConfigurationSource.

@Choc13
Copy link
Collaborator Author

Choc13 commented Jun 8, 2020

@davidalpert I believe you are correct. This would break the ConsulConfigurationSource interface as you mentioned. I hadn't put a lot of thought in this yet as I was going to leave it until the next major change, which I usually do alongside a major ASP.NET Core release, so that the versions of this library match those of the ASP.NET Core major version.

My thoughts about the implementation when I created this issue were that I would just make direct Flurl calls. I think that will be quite straight forward as I believe it is the Consul server that handles the long polling through the index it returns on the response, and we already have the client side mechanism through the _lastIndex field. I think for the KV part of the consuldotnet client it's just a straight forward REST Api.

I think the tricky part comes in around the ConsulConfigurationSource area where we might want to think about how much control we give clients over the configurability of the API calls. For instance there is currently #100 asking about how Polly retry policies could be incorporated. So this rewrite could give us a chance to factor that in appropriately.

If you wanted to have a crack at this please feel free. I won't be getting around to it for a while.

@kewinbrand
Copy link

kewinbrand commented Jun 9, 2020

hey @Choc13

i found a promising new fork from https://github.com/PlayFab/consuldotnet.
it is https://github.com/G-Research/consuldotnet. they also already publised a new version in nuget.
what do you think?

thank you

@Choc13
Copy link
Collaborator Author

Choc13 commented Jun 9, 2020

@kewinbrand Thanks for that.

I actually came across it myself today funnily enough. I’ll review it and have a think. Partly I think the removal of a dependency is worth considering if we only need a small part of it which is just a thin wrapper around a REST api. However in the first instance I’ll probably move over to their version as it should be easy and will ensure things can remain up to date in the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants