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

Move the Maybe Datacenter into the ConsulClient, drop from requests #33

Merged
merged 4 commits into from
Apr 7, 2021

Conversation

ketzacoatl
Copy link
Collaborator

This update move the Datacenter into the ConsulClient data structure. In effect, we're saying a "client connection" specifies the datacenter. So if you want to interact with multiple datacenters in a consul cluster, you would have multiple clients. I think this is a good thing, as MOST of the time, there's a single datacenter we're targeting, and if/when there are multiple, having separate clients is a great way to keep the code clean and obvious.

With this change, the datacenter doesn't need to be passed around outside the client, we can simply rely on the client data structure to contain the info we need for the interaction with consul.

With this update, users will need to change their calling code to use the library (dropping the Maybe Datacenter from most functions, and specifying it when initializing the client). An example of that update looks like:

+dc1 :: Datacenter
+dc1 = Datacenter "dc1"
+
 newClient :: IO ConsulClient
-newClient = initializeConsulClient "localhost" consulPort emptyHttpManager
+newClient = initializeConsulClient "localhost" consulPort dc1 emptyHttpManager
 testGetInvalidKey :: TestTree
 testGetInvalidKey = testCase "testGetInvalidKey" $ do
   client@ConsulClient{..} <- newClient
-  x <- getKey client "nokey" Nothing Nothing Nothing
+  x <- getKey client "nokey" Nothing Nothing
   assertEqual "testGetInvalidKey: Found a key that doesn't exist" x Nothing

While we will want to update async to withAsync as well, these changes will result in a version bump to 0.6.0.

This contributes to #32.

@ketzacoatl ketzacoatl requested a review from nh2 December 10, 2020 13:56
@ketzacoatl ketzacoatl self-assigned this Dec 10, 2020
@ketzacoatl
Copy link
Collaborator Author

I forgot to mention, tests are still passing:

image

@ketzacoatl
Copy link
Collaborator Author

Here is the failure from Travis:

image

I'm not sure if that is a transient error or if there's an update to be made to the API call.

@ketzacoatl
Copy link
Collaborator Author

With consul 1.9.0, two (session) tests fail:

image

That should probably be fixed separately.

@ketzacoatl
Copy link
Collaborator Author

All those issues noted above have been resolved (see #36 and #38), and this PR has been updated/rebased. I also have a version of this PR that is based on #39, though I'm not sure that one is good to merge, so I haven't pushed it. @nh2, LMK if I should update this after merging that PR, or if this should be first.

@ketzacoatl
Copy link
Collaborator Author

Tests are passing:

image

Copy link
Collaborator

@nh2 nh2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your PR description is great, we should copy it into the changelog when we make this API backwards incompatble release.

The only thing you might want to add is that it is still very easy to provide a custom datacenter by updating the client record:

-  x <- getKey client "nokey" Nothing Nothing Nothing
+  x <- getKey client{ ccDatacenter = Datacenter "mydc" } "nokey" Nothing Nothing

I've made a small change request to maintain the Maybe-ness of the ccDatacenter.

src/Network/Consul/Types.hs Outdated Show resolved Hide resolved
tests/Main.hs Outdated Show resolved Hide resolved
@ketzacoatl
Copy link
Collaborator Author

Your PR description is great, we should copy it into the changelog when we make this API backwards incompatble release.

The only thing you might want to add is that it is still very easy to provide a custom datacenter by updating the client record:

-  x <- getKey client "nokey" Nothing Nothing Nothing
+  x <- getKey client{ ccDatacenter = Datacenter "mydc" } "nokey" Nothing Nothing

I've made a small change request to maintain the Maybe-ness of the ccDatacenter.

This should now be complete.

@ketzacoatl
Copy link
Collaborator Author

OK, #39 was updated and merged, so I've rebased this PR and confirmed tests are passing. This also addresses the Maybe Datacenter request, so I believe this is ready for re-review and merge.

@ketzacoatl
Copy link
Collaborator Author

@nh2 maybe you can review? I am blocked without a thumbs up or more changes to make.

@ketzacoatl ketzacoatl force-pushed the move-dc-into-consul-client branch 4 times, most recently from 6f66a50 to e72839b Compare April 7, 2021 19:33
We can still declare a Datacenter to use when initializing our client:

```
x <- getKey
       client{ ccDatacenter = Datacenter "mydc" }
       "nokey"
       Nothing
       Nothing
```
Don't wait, we need it now as part of the v0.6.0 release's breaking change.
We'll continue to update the CHANGELOG with entries for previous releases.
@ketzacoatl
Copy link
Collaborator Author

After fixing CI for #50, I bumped the version, added a CHANGELOG, updated the tests to work with #48, and made sure CI is still looking good. I believe this is now ready to merge.

@ketzacoatl ketzacoatl merged commit 332dfed into master Apr 7, 2021
@ketzacoatl ketzacoatl deleted the move-dc-into-consul-client branch April 7, 2021 20:11
This was referenced Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants