Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Allow default alert contact to be configured #21

Merged
merged 4 commits into from
May 23, 2019

Conversation

aequitas
Copy link
Contributor

@aequitas aequitas commented Mar 30, 2019

  • Convert alert contact ID to string (it can be 0 padded number)
  • Add alert contact datasource
  • Extend example configuration with default alert contact

TODO:

  • Pagination
  • Migration code for int to string conversion alert contact id
  • Tests

Copy link
Contributor

@drubin drubin left a comment

Choose a reason for hiding this comment

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

Thanks for this I would love to use this as well.

Just a few questions :)

uptimerobot/api/alert_contact.go Outdated Show resolved Hide resolved
@aequitas
Copy link
Contributor Author

aequitas commented Apr 3, 2019

I added a configuration example of what I think this will look like when fully implemented. I'll look into the pagination stuff as well.

@aequitas
Copy link
Contributor Author

aequitas commented Apr 3, 2019

The latest commit is a working example. I still need to add pagination support.

I had to convert the alert contact ID from int to string as the value can be a zero padded number and the correct alert contact will not be found without those zero's. Maybe this applies to the other resources as well? Also I have not tested this with an existing configuration so migration code might need to be added as well.

@aequitas aequitas force-pushed the default-alert-contact branch 3 times, most recently from 61c3fee to afb542a Compare April 4, 2019 19:59
@aequitas
Copy link
Contributor Author

aequitas commented Apr 4, 2019

So as far as I can tell there is no reason for migration, as Terraform stored the ID's as strings in the state file. So by reading them as string instead of int in the new code they are automatically read correctly.

@aequitas aequitas marked this pull request as ready for review April 4, 2019 20:01
@@ -94,12 +144,12 @@ func (client UptimeRobotApiClient) CreateAlertContact(req AlertContactCreateRequ
return
}

return client.GetAlertContact(int(alertcontact["id"].(float64)))
return client.GetAlertContact(alertcontact["id"].(string))
Copy link
Contributor

@leeif leeif Apr 9, 2019

Choose a reason for hiding this comment

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

The alertcontact["id"] here seems to be a float64 type which can't be converted by .(string).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the API documentation the ID is a string https://uptimerobot.com/api

image

I stumbled upon this because one of the ID's generated for me by the API started with a 0 which would be stripped when converting form a string to a int/float64 and thus Terraform would record an invalid ID in the state file.

Copy link
Contributor

@leeif leeif Apr 10, 2019

Choose a reason for hiding this comment

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

May be this is a uptimerobot issue. At least for me, the response of this API is different from the document.
https://gist.github.com/leeif/0bc4eb5f9ca03b9c487169d98167cc40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is an issue at UR's end.

Alert contact create returns a integer:

$ curl -sS -X POST -H "Cache-Control: no-cache" -H "Content-Type: application/x-www-form-urlencoded" -d "api_key=XXXX&format=json&friendly_name=Test2&type=2&value=test2%40example.com"  "https://api.uptimerobot.com/v2/newAlertContact"|jq .
{
  "stat": "ok",
  "alertcontact": {
    "id": 2809628
  }
}

While getAlertContacts returns a string:

$ curl -sS -X POST -H "Cache-Control: no-cache" -H "Content-Type: application/x-www-form-urlencoded" -d "api_key=XXXXXX&format=json"  "https://api.uptimerobot.com/v2/getAlertContacts"|jq .
{
  "stat": "ok",
  "offset": 0,
  "limit": 50,
  "total": 3,
  "alert_contacts": [
    {
      "id": "0717807",
      "friendly_name": "[email protected]",
      "type": 2,
      "status": 2,
      "value": "[email protected]"
    },
    {
      "id": "2806958",
      "friendly_name": "Test",
      "type": 2,
      "status": 0,
      "value": "[email protected]"
    },
    {
      "id": "2809628",
      "friendly_name": "Test2",
      "type": 2,
      "status": 0,
      "value": "[email protected]"
    }
  ]
}

One thing I do notice is that the ID of the default alert contact starts with 0 but the others are in a completely different number range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also querying without the zero does not work:

$ curl -sS -X POST -H "Cache-Control: no-cache" -H "Content-Type: application/x-www-form-urlencoded" -d "api_key=XXXXX&format=json&alert_contacts=717807"  "https://api.uptimerobot.com/v2/getAlertContacts"|jq .
{
  "stat": "fail",
  "error": {
    "type": "not_found",
    "message": "Alert Contact(id: 717807) not found."
  }
}
$ curl -sS -X POST -H "Cache-Control: no-cache" -H "Content-Type: application/x-www-form-urlencoded" -d "api_key=XXXXX&format=json&alert_contacts=0717807"  "https://api.uptimerobot.com/v2/getAlertContacts"|jq .
{
  "stat": "ok",
  "offset": 0,
  "limit": 50,
  "total": 1,
  "alert_contacts": [
    {
      "id": "0717807",
      "friendly_name": "[email protected]",
      "type": 2,
      "status": 2,
      "value": "[email protected]"
    }
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I added a conversion to string instead of casting. I don't think UR will change their API and break all clients out there (didn't attempt to contact them though). But this should cover both cases:

  • in newAlertContact where the ID is returned as a integer
  • in all other cases where it is handled as a string

Also only the response values are types (due to JSON) and the request values not (as they are HTTP parameters).

@louy louy added the enhancement New feature or request label Apr 11, 2019
@louy
Copy link
Owner

louy commented Apr 23, 2019

Nice work @aequitas!

Is this ready to be merged? Do you still want to add a test?

Johan Bloemberg added 4 commits April 23, 2019 20:17
- Convert alert contact ID to string (it can be 0 padded number)
- Add alert contact datasource
- Extend example configuration with default alert contact
@aequitas
Copy link
Contributor Author

@louy I added a test and fixed some bugs I found when running the other acceptance tests. I also did a small testrun 'converting' a configuration created using the latest master to one with this branch and found no issues. So afaict this now works as expected and can be merged.

@louy louy merged commit 29d4e17 into louy:master May 23, 2019
@louy louy mentioned this pull request May 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants