Re-implement Contacts and Teams

[joshsouza]

When I initially worked on #57 I believe that Pingdom had not added the alerting API (Teams and Contacts) to v3.1 (I could be mistaken, but 🤷). As a result I deleted the old code for managing users and teams.

After reviewing the state of the world yesterday, it appears that we could re-add this functionality. However the API is pretty vastly different than before, and requires proper JSON (not query parameters) for submission.
I've taken a crack at solving this, trying to stay as close to the lines given by the rest of the project, while enabling JSON-style POST/PUT requests where needed.

To be clear: This is one day of work, and likely needs some tweaking/improvement before we should merge it in, but the acceptance tests work, and I think it's a good starting point.

Things I think we should discuss:

1. How I handled the JSON POST/PUT stuff
2. The lack of validation (In particular, I found that provider on SMSNotification stuff is very picky in their api) is probably not great, but was more than I was able to get to in a day
3. I've got some pretty repetitive structs (SMSNotification vs SMSNotificationResponse) that I tried to model off of what was already in project, but we could likely simplify for useability.
4. The Create calls don't update the passed in object to add the ID automatically. This seems to follow existing trends, but I would think that we might want to do that so that users can do pingdom.Contacts.Create(&contact) and subsequently contact.ID
5. I updated some casing stuff for go-lint, but that may not be the direction you'd like to take this. We can easily drop that commit.

@cce - You may be interested in this PR

[joshsouza]

Fun fact: Yesterday on https://docs.pingdom.com/api/#tag/Contacts/paths/~1alerting~1contacts/post there were references to SMSes Emails APNS and AGCN notification types. Today there's only SMSes and Emails. So they're doing something on the pingdom side.
I'm fine carving that code back out if we feel it's not going to be used.

[russellcardullo]

there were references to SMSes Emails APNS and AGCN notification types. Today there's only SMSes and Emails. So they're doing something on the pingdom side.

I too only see SMSes and Emails on the schema for creating contacts. But looking at their linked OpenAPI spec I see APNS and AGCM but only on the ContactTargets for reading. I'm assuming this is a read only parameter for the API and we can return on read but ignore on create?

https://docs.pingdom.com/api/API_3.1.yaml

    ContactTargets:
      ...
        notification_targets:
          anyOf:
            - $ref: '#/components/schemas/SMSes'
            - $ref: '#/components/schemas/Emails'
            - $ref: '#/components/schemas/APNS'
            - $ref: '#/components/schemas/AGCM'

[russellcardullo]

The lack of validation (In particular, I found that provider on SMSNotification stuff is very picky in their api)

I think we can probably ignore lack of validation for SMSNotication.provider. I'd be worried that if we validated client side we'd be out of sync with acceptable values on the pingdom API.

I've got some pretty repetitive structs ...but we could likely simplify for useability.

Yeah originally some of the duplication came from mismatch in request vs response in the old Pingdom 2.X API. I see some of that has carried over to the 3.1 API (for instance Check.Host on PUT but Check.Hostname on GET). In this case though I think we could probably have a single struct in cases where the values are the same.

If we can, I'd try to follow their Open API spec wherever possible, and try to keep a 1-1 mapping between the types they define there and the types we have here.

In fact, if we were inclined since they have an OpenAPI spec we could always replace this entire codebase with a client generated using openapi-generator but that'd be a breaking change for sure. Maybe for the next version!

The Create calls don't update the passed in object to add the ID automatically.

Yeah we might want to change that at some point but I'd say out of scope here right now. I'd probably be on the side of changing the various methods to no longer take in a pointer, especially given the mismatch the Pingdom API has betwen certain request response types. For instance change this:

newCheck := pingdom.HttpCheck{Name: "Test Check", Hostname: "example.com", Resolution: 5}
check, err := client.Checks.Create(&newCheck)

to

newCheck := pingdom.HttpCheck{Name: "Test Check", Hostname: "example.com", Resolution: 5}
check, err := client.Checks.Create(newCheck)

I think that might make it more clear that the two types are different since the returned check will have different members from the parameter check beyond just the ID.

Again, that's a breaking change we might consider later so for now I think it's fine to follow the existing pattern.

I updated some casing stuff for go-lint, but that may not be the direction you'd like to take this. We can easily drop that commit.

Ah I should fix the lint rules to ignore the casing for now. I'd much prefer the way you have it but we'll probably want to make a separate PR for that as well as some other breaking changes.

[joshsouza]

@russellcardullo I went ahead and dropped the commit with the lint stuff, and simplified all the type definitions for contacts (Turns out there are differences in the Team stuff, so the response types make sense there).

Given that the APNS and AGCM stuff is still in the API spec, I agree it makes sense to leave it in place for now.

I did find out something rather unfortunate with their API (and should probably report a bug, now that I'm saying it): When you submit a 'SMSNotification', to use "Bulk SMS", the API rejects anything that is not bulk sms (note the space). However, using the web UI submits bulksms (to a different endpoint). The result is that setting bulk sms ends up being invalid (I don't know whether it works or not, but it breaks the Web UI's rendering). Not something I think we want to fix on this end, but an unfortunate bug I encountered when testing this with the terraform provider.

I think this is ready for full-on review, and I'd be happy to make any changes you think it needs.

I'm almost done prepping a PR for the pingdom provider for Terraform, and I'll be using this branch as my expected future state for that work.

[cce]

Hi @joshsouza I double-checked and APNS and AGCM notification types are currently only created in Pingdom's iOS and Android apps when you enable push notifications, so they are only available as GET methods in the Pingdom API — so they are read-only as @russellcardullo observed.

As for the bulksms issue thanks for discovering this! Will get back to you on that.

[joshsouza]

Great! Thanks @cce
So we can probably leave the APNS and AGCM code as is, or maybe update it to filter them out on PUT/POST. 🤔

[cce]

@joshsouza re: the other issue you found, the API was updated this morning to use and validate bulksms as the correct value.

[joshsouza]

@cce Awesome! Thanks for that!

[joshsouza]

@russellcardullo Any chance you'll have time to review this PR this week?

[russellcardullo]

Hi @joshsouza, I should be able to take a look in the next few days. Thanks for this!

[russellcardullo]

This looks great @joshsouza. Thanks again!