search

uptime-com / terraform-provider-uptime

A Terraform provider that provisions checks via the Uptime.com REST API
MIT License
20 stars 18 forks source link

probe-checks: add icmp check support #26

Closed pinotelio closed 1 year ago

pinotelio commented 1 year ago

This commit adds the support of ICMP checks/probes.

Signed-off-by: pinotelio ahmadreza.mollapour@gmail.com

pinotelio commented 1 year ago

PTAL: @kylegentle @liamkinne

liamkinne commented 1 year ago

@pinotelio Code looks good to me. Don't have the facilities to to test against the actual API. I don't have write access and likely neither does Kyle so you may have to raise a support request to get one of the Uptime guys to look at this.

mikluko commented 1 year ago

@pinotelio LGTM, but it requires acceptance tests coverage. Check out *_test.go for reference.

pinotelio commented 1 year ago

@pinotelio LGTM, but it requires acceptance tests coverage. Check out *_test.go for reference.

@mikluko Please take a look. The test coverage and proper docs have been added.

pinotelio commented 1 year ago

Any update guys? @mikluko @liamkinne @kylegentle

kylegentle commented 1 year ago

@pinotelio, thanks for contributing this improvement!

I haven't looked at the project in a long time, and unfortunately I don't have access to approve or merge PRs. Mikhail seems like the right person to approve.

On a separate note, it would be helpful for someone from Uptime to flesh out the Contribution Guidelines section in the README, and set expectations for how PRs will be handled.

bazzisoft commented 1 year ago

@pinotelio We'll do our best to look at this next week, there's a bit of a tricky situation where we're on the verge of releasing a major update v2.0 of the provider (it's been in beta for a while) which has some very significant code changes, so we'll need to check how we can port this over to that too. We'll keep you posted.

@kylegentle Thanks for your comment & good idea re the guidelines, we'll work on getting those into the README!

pinotelio commented 1 year ago

@pinotelio We'll do our best to look at this next week, there's a bit of a tricky situation where we're on the verge of releasing a major update v2.0 of the provider (it's been in beta for a while) which has some very significant code changes, so we'll need to check how we can port this over to that too. We'll keep you posted.

@kylegentle Thanks for your comment & good idea re the guidelines, we'll work on getting those into the README!

Since version 2.0 has released a couple of weeks ago, it's probably best to close this pull request. I'll take a look at the fresh code changes and submit a new pull request incorporating the latest updates.