deregisterService uses PUT method instead of GET

[wraithm]

Previously, the deregisterService used a GET request to the deregister endpoint. Consul requires that this must be a PUT request now. This change converts the request into a PUT request using the createRequest convention that empty string request bodies are PUT requests.

This change also adds a test for deregisterService. A service is created, then destroyed, and confirmed that the service was removed.

This change also fixes a minor bug in the testRegisterService. It did not check that the list returned wasn't empty. If the list of services is empty, it also wasn't created.

Closes #47

[wraithm]

I also added a datacenter parameter to the deregisterService function. I don't know if this is right. I'd be happy to remove that change if you don't think it's right.

Of course, this is a breaking change to the API. Though, I imagine not that many people are using this function because it was broken 😂

[wraithm]

I've tested this on my local machine. It looks like CI is broken...

[wraithm]

Any thoughts on this @ketzacoatl ?

[ketzacoatl]

Hi @wraithm,

Thank you for the PR and write up. I was away for the weekend and am only now seeing the notifications. I have been wanting to get back into dev here, so it's nice to have the nudge :)

You raise some good questions. It has been quite some time since I last reviewed the details, and I'd like a couple days to look them over again, but here are a couple initial thoughts:

• I'd like to find out which version of consul this change came in, or if it's always been PUT.
• Depending on that, we may want to make a release before this change.
• CI should be working, IDK why it isn't, that was good before.
• We'll want to run tests on a few versions of consul, though we also haven't run on the most recent releases, will need to update CI to include those.
• There is a PR for changing how the dc is handled, though I don't think we need to make this dependent on that.
• Thank you for adding the test, that's great!
• Do you have any particular interest in the library or projects you would use it for? Feedback on the directions we take next is useful, feel free to submit other issues with that feedback.
• Once this is merged, we'll want to do a version bump and release.

[ketzacoatl]

OK, AFAICT, the deregister API has always been PUT.

Here is the olded version we still test the library with:

$ git checkout v1.3.1
$ git grep deregister | grep PUT
CHANGELOG.md:    | /v1/agent/check/deregister | PUT |
CHANGELOG.md:    | /v1/agent/service/deregister | PUT |
CHANGELOG.md:    | /v1/catalog/deregister | PUT |
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("PUT", "/v1/agent/check/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("PUT", "/v1/agent/check/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("PUT", "/v1/agent/check/deregister/test?token=root", nil)
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("PUT", "/v1/agent/service/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("PUT", "/v1/agent/service/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("PUT", "/v1/agent/service/deregister/test?token=root", nil)
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("PUT", "/v1/agent/service/deregister/test-id", nil)
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("PUT", "/v1/agent/service/deregister/"+proxyID, nil)
agent/catalog_endpoint_test.go: req, _ := http.NewRequest("PUT", "/v1/catalog/deregister", jsonReader(args))
agent/http_oss.go:  registerEndpoint("/v1/agent/check/deregister/", []string{"PUT"}, (*HTTPServer).AgentDeregisterCheck)
agent/http_oss.go:  registerEndpoint("/v1/agent/service/deregister/", []string{"PUT"}, (*HTTPServer).AgentDeregisterService)
agent/http_oss.go:  registerEndpoint("/v1/catalog/deregister", []string{"PUT"}, (*HTTPServer).CatalogDeregister)
api/agent.go:   r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID)
api/agent.go:   r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+checkID)
api/catalog.go: r := c.c.newRequest("PUT", "/v1/catalog/deregister")
website/source/api/agent/check.html.md:| `PUT`  | `/agent/check/deregister/:check_id` | `application/json`         |
website/source/api/agent/service.html.md:| `PUT`  | `/agent/service/deregister/:service_id` | `application/json` |
website/source/api/catalog.html.md:| `PUT`  | `/catalog/deregister`        | `application/json`         |
website/source/docs/guides/external.html.md:$ curl -X PUT -d '{"Datacenter": "dc1", "Node": "google"}' http://127.0.0.1:8500/v1/catalog/deregister
website/source/docs/upgrade-specific.html.md:| /v1/agent/check/deregister | PUT |
website/source/docs/upgrade-specific.html.md:| /v1/agent/service/deregister | PUT |
website/source/docs/upgrade-specific.html.md:| /v1/catalog/deregister | PUT |

Here's an even older version:

$ git checkout v0.7.5
Previous HEAD position was 1ee4f6582 Release v1.0.8
HEAD is now at 21f2d5ad0 Release v0.7.5
$ git grep deregister | grep PUT
api/agent.go:   r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID)
api/agent.go:   r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+checkID)
api/catalog.go: r := c.c.newRequest("PUT", "/v1/catalog/deregister")
website/source/docs/agent/http/catalog.html.markdown:The deregister endpoint expects a JSON request body to be `PUT`. The request
website/source/docs/guides/external.html.markdown:$ curl -X PUT -d '{"Datacenter": "dc1", "Node": "google"}' http://127.0.0.1:8500/v1/catalog/deregister

That is nice.. and simplifies our next steps.

[wraithm]

OK, AFAICT, the deregister API has always been PUT.

I believe it's always accepted PUT, but it used to also accept GET, and then they deprecated that.

[ketzacoatl]

I believe it's always accepted PUT, but it used to also accept GET, and then they deprecated that.

Ah, yes, good catch!

I had searched for GET in v1.3.1 and more recent versions.

The latest version I could find GET for deregister was in v0.9.4:

$ git checkout v0.9.4
Previous HEAD position was 2c7715154 Release v0.8.5
HEAD is now at 40f243a5d Release v0.9.4
$ git grep deregister | grep GET
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("GET", "/v1/agent/check/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("GET", "/v1/agent/check/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("GET", "/v1/agent/check/deregister/test?token=root", nil)
agent/agent_endpoint_test.go:   req, _ := http.NewRequest("GET", "/v1/agent/service/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("GET", "/v1/agent/service/deregister/test", nil)
agent/agent_endpoint_test.go:       req, _ := http.NewRequest("GET", "/v1/agent/service/deregister/test?token=root", nil)
agent/catalog_endpoint_test.go: req, _ := http.NewRequest("GET", "/v1/catalog/deregister", jsonReader(args))

And it's gone in 1.0:

$ git checkout v1.0.0
Previous HEAD position was 40f243a5d Release v0.9.4
HEAD is now at 51ea240df Release v1.0.0
$ git grep deregister | grep GET
$

Given that it's not in 1.0, I don't think we need to worry about supporting it.

[wraithm]

Btw, it looks like several endpoints in the consul-haskell package had the same change: they used to make GET reqs, then they got changed to PUT. It just looks like deregister got missed somehow.

I used the function that the other requests use to make PUT reqs. This GET -> PUT requirement has clearly been an issue in the past for this lib.

[ketzacoatl]

Yea, it was probably missed due to not having tests. That's why getting tests for each implemented API has been a priority, though that effort then got bogged down in consistency and ergonomic updates. It's just a bunch of work to slug through :)

Thanks for the help with this so far!

[ketzacoatl]

OK, I've fixed CI (see #50). We should now rebase this branch to see the new tests run.

@wraithm, do you feel comfortable doing that git fetch -a && git rebase origin/master master? I don't mind helping, but it'd be easier for you in your position, to update the PR.

[wraithm]

Yeah, I'll take care of that in 2 secs

[ketzacoatl]

Fantastic! Do you want to bump the lib version to 0.5.1 in the cabal file in the PR? I can also do it after the merge, just LMK.

[ketzacoatl]

Thank you for the contribution!

[wraithm]

Thanks!