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

Adding support to HeartBeat Monitor type #18

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rogerioefonseca
Copy link

@rogerioefonseca rogerioefonseca commented Apr 16, 2022

  • Add support to heartbeat
  • Update documentation
  • Refactor to reduce number of lines
  • test the new monitor type

close #14

@rogerioefonseca
Copy link
Author

@mnaser PTAL on this new MR.
I was working in a fork from louy repository and that one was some commits behind yours, now, I forked yours and created a new MR based on this.
And then the conflicts were gone

Copy link
Member

@mnaser mnaser left a comment

Choose a reason for hiding this comment

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

one tiny nit but the refactor looks great.

Comment on lines 43 to 50
* `http_method` - available for HTTP monitoring. Can be one of the following:
- `HEAD` (default for non-keyword)
- `GET` (default for keyword)
- `POST`
- `PUT`
- `PATCH`
- `DELETE`
- `OPTIONS`
Copy link
Member

Choose a reason for hiding this comment

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

I think this was accidentally swallowed in the refactor :)

Copy link
Author

Choose a reason for hiding this comment

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

Yep, you are right, thanks for the review.
Fixed - 8452343

@mnaser
Copy link
Member

mnaser commented Apr 19, 2022

It looks like the tests failed, could you perhaps try and run them locally and see if you can reproduce and fix this?

I appreciate your follow up on this!

@rogerioefonseca
Copy link
Author

It looks like the tests failed, could you perhaps try and run them locally and see if you can reproduce and fix this?

I appreciate your follow up on this!

I'll do it later today.

@rogerioefonseca
Copy link
Author

rogerioefonseca commented Apr 20, 2022

It looks like the tests failed, could you perhaps try and run them locally and see if you can reproduce and fix this?
I appreciate your follow up on this!

I'll do it later today.

Done 9b5e31c

mnaser added 2 commits April 21, 2022 10:15
The tests were failing since we were making use of the same name.
@rogerioefonseca
Copy link
Author

rogerioefonseca commented Apr 21, 2022

It looks like the tests failed, could you perhaps try and run them locally and see if you can reproduce and fix this?
I appreciate your follow up on this!

I'll do it later today.

It was super late, and I misread the test outputs, now that I saw your commit I realized the tests problems.
But nice that you figure those and applied to master. I rebased and added your commits to this MR, so probably the tests will run without errors.

BTW, is there a special reason for the tests do not run right after the pull request is created?
I saw in the workflow that the tests are limited to be executed based on a cron definition...

@mnaser
Copy link
Member

mnaser commented Apr 22, 2022

BTW, is there a special reason for the tests do not run right after the pull request is created?
I saw in the workflow that the tests are limited to be executed based on a cron definition...

It's a GitHub thing since you have not had any contributions yet.

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

@rogerioefonseca
Copy link
Author

rogerioefonseca commented Apr 25, 2022

BTW, is there a special reason for the tests do not run right after the pull request is created?
I saw in the workflow that the tests are limited to be executed based on a cron definition...

It's a GitHub thing since you have not had any contributions yet.

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Looks like it needs to add the TF and API KEY env variables into the Repo configuration, for some reason that I do not know looks like those are not being loaded into this one.

Will you have time to TAL on this?
Let me know if I can help somehow.

@rogerioefonseca
Copy link
Author

BTW, is there a special reason for the tests do not run right after the pull request is created?

I saw in the workflow that the tests are limited to be executed based on a cron definition...

It's a GitHub thing since you have not had any contributions yet.

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Looks like it needs to add the TF and API KEY env variables into the Repo configuration, for some reason that I do not know looks like those are not being loaded into this one.

Will you have time to TAL on this?

Let me know if I can help somehow.

Ping...
🙂

@FlxPeters
Copy link

Any progress here? Would like to use this feature too and this seams to be the best maintained fork of the original repo.
Let me know if i can help bring this to an successful merge.

@rogerioefonseca
Copy link
Author

Any progress here? Would like to use this feature too and this seams to be the best maintained fork of the original repo.

Let me know if i can help bring this to an successful merge.

@mnaser ....

@rogerioefonseca
Copy link
Author

Any progress here? Would like to use this feature too and this seams to be the best maintained fork of the original repo.

Let me know if i can help bring this to an successful merge.

Not sure what else can I do as well.
@mnaser, I'm willing to help to get this merged as well.

@rogerioefonseca
Copy link
Author

Any progress here? Would like to use this feature too and this seams to be the best maintained fork of the original repo.

Let me know if i can help bring this to an successful merge.

Not sure what else can I do as well.
@mnaser, I'm willing to help to get this merged as well.

Ping...

@yktakaha4
Copy link

@rogerioefonseca I also sent a PR to this repository, but since they seem to be busy, we decided to manage our own provider.
Heartbeat automation would be great for us, so please send me a PR if you'd like.
https://github.com/lapras-inc/terraform-provider-uptimerobot

@rogerioefonseca
Copy link
Author

@rogerioefonseca I also sent a PR to this repository, but since they seem to be busy, we decided to manage our own provider. Heartbeat automation would be great for us, so please send me a PR if you'd like. https://github.com/lapras-inc/terraform-provider-uptimerobot

Nice, I'll do it on the weekend.

@dcardellino
Copy link

Any Update here? Would like to use this feature to not do this manually...

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.

Support monitor type "heartbeat"
5 participants