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

Enable self-hosted healthchecks.io instances #37

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Penagwin
Copy link

@Penagwin Penagwin commented Sep 7, 2023

Issues #27 and #32 address supporting self-hosted instances.
This PR is a continuation of #32

Notably line plugins/module_utils/healthchecksio.py#349 needed to have the api_base_url added to allow checks to be made.

dsander and others added 7 commits September 13, 2022 00:53
By specifying `api_base_url` users can connect to their own self-hosted
instances.
The UUID is always the last component of the URL. For self-hosted
instances the ping url is `<scheme>://<domain>/ping/<uuid>` on
healthchecks.io it is `https://hc-ping.com/<uuid>`.
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a963765) 74.66% compared to head (45f7b7e) 74.47%.

❗ Current head 45f7b7e differs from pull request most recent head d183be0. Consider uploading reports for the commit d183be0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
- Coverage   74.66%   74.47%   -0.20%     
==========================================
  Files           8        8              
  Lines         375      380       +5     
  Branches       58       59       +1     
==========================================
+ Hits          280      283       +3     
- Misses         54       55       +1     
- Partials       41       42       +1     
Flag Coverage Δ
integration 74.47% <75.00%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
plugins/module_utils/healthchecksio.py 66.39% <75.00%> (-0.14%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@mamercad mamercad 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 seeing this through, a few minor changes please!

@Penagwin
Copy link
Author

I updated the documentation and the api url, I'm unsure about the CI failing

@mamercad
Copy link
Collaborator

I updated the documentation and the api url, I'm unsure about the CI failing

Thanks, I'll have a look.

@mamercad
Copy link
Collaborator

I updated the documentation and the api url, I'm unsure about the CI failing

Thanks, I'll have a look.

You'll have to dig a bit harder on this, here's main:

❯ ansible-test integration --python 3.9 badges_info
Running badges_info integration test role

PLAY [testhost] ****************************************************************

TASK [Gathering Facts] *********************************************************
ok: [testhost]

TASK [badges_info : Ensure API key is provided] ********************************
skipping: [testhost]

TASK [badges_info : Get the project badges] ************************************
ok: [testhost]

TASK [badges_info : Verify project badges] *************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

PLAY RECAP *********************************************************************
testhost                   : ok=3    changed=0    unreachable=0    failed=0    skipped=1    rescued=0    ignored=0

Here's this branch:

healthchecksio-py3.9 ❯ gh pr checkout 37
Switched to branch 'Penagwin/main'

healthchecksio Penagwin/main*​ 2s 
healthchecksio-py3.9 ❯ ansible-test integration --python 3.9 badges_info
Running badges_info integration test role

PLAY [testhost] ****************************************************************

TASK [Gathering Facts] *********************************************************
ok: [testhost]

TASK [badges_info : Ensure API key is provided] ********************************
skipping: [testhost]

TASK [badges_info : Get the project badges] ************************************
fatal: [testhost]: FAILED! => {"changed": false, "msg": "Failed to get badges [HTTP 400: (empty error message)]"}

PLAY RECAP *********************************************************************
testhost                   : ok=1    changed=0    unreachable=0    failed=1    skipped=1    rescued=0    ignored=0   

NOTICE: To resume at this test target, use the option: --start-at badges_info
FATAL: Command "ansible-playbook badges_info-oswog6_j.yml -i inventory" returned exit status 2.

@Penagwin
Copy link
Author

Penagwin commented Oct 19, 2023

I apologize I haven't had a ton of time to check.

HTTP 400 so we're hitting a server. Interestingly the /api/v1/ api will 404 for invalid urls, the ping api gives 400

Based on their docs https://healthchecks.io/docs/apiv1/

They use two different urls, one for pings and one for management.

https://hc-ping.com/ and https://healthchecks.io/api/v1/

We're using this to self host, and it looks like we are using the different endpoint (we use /ping/ instead of a different domain) but we're doing it in a bash script which is why ansible isn't having issues with it.

I didn't realize there was two endpoints. So it looks like I'll need to add a second api endpoint for the pings themselves.

EDIT: I just realized a separate ping key will also be needed

@mamercad
Copy link
Collaborator

I apologize I haven't had a ton of time to check.

HTTP 400 so we're hitting a server. Interestingly the /api/v1/ api will 404 for invalid urls, the ping api gives 400

Based on their docs https://healthchecks.io/docs/apiv1/

They use two different urls, one for pings and one for management.

https://hc-ping.com/ and https://healthchecks.io/api/v1/

We're using this to self host, and it looks like we are using the different endpoint (we use /ping/ instead of a different domain) but we're doing it in a bash script which is why ansible isn't having issues with it.

I didn't realize there was two endpoints. So it looks like I'll need to add a second api endpoint for the pings themselves.

EDIT: I just realized a separate ping key will also be needed

Again, thanks for digging into this, I appreciate it.

@mamercad
Copy link
Collaborator

I apologize I haven't had a ton of time to check.
HTTP 400 so we're hitting a server. Interestingly the /api/v1/ api will 404 for invalid urls, the ping api gives 400
Based on their docs https://healthchecks.io/docs/apiv1/
They use two different urls, one for pings and one for management.
https://hc-ping.com/ and https://healthchecks.io/api/v1/
We're using this to self host, and it looks like we are using the different endpoint (we use /ping/ instead of a different domain) but we're doing it in a bash script which is why ansible isn't having issues with it.
I didn't realize there was two endpoints. So it looks like I'll need to add a second api endpoint for the pings themselves.
EDIT: I just realized a separate ping key will also be needed

Again, thanks for digging into this, I appreciate it.

Also, I toyed with running a instance in a workflow, might be worth considering for this PR.

@Penagwin
Copy link
Author

@mamercad This is not finished, but I wasn't sure how you'd want the separate ping/management api's organized so I was wondering if this looked like a decent method.

Related - I'm using this at work and I really need access to the management api v3's slug field. Basically I need to be able to set the slug field during creation which you can't do with the v1 api.

Currently I think this module uses the v1 api. The only breaking change is that v2 seperated the "started" from the "status"

{"status": "started", ...}
Management API v2 would report its status as:
{"status": "down", "started": true, ...}

Adding the slug field to the creation just requires adding it as an optional argument I think?
I'm not sure how you want to handle the v2 change. I did a quit look earlier and didn't see anything referring to "started" so it would only be a breaking change for the actual playbooks that use the module, not within the module itself.

@mamercad
Copy link
Collaborator

@mamercad This is not finished, but I wasn't sure how you'd want the separate ping/management api's organized so I was wondering if this looked like a decent method.

Related - I'm using this at work and I really need access to the management api v3's slug field. Basically I need to be able to set the slug field during creation which you can't do with the v1 api.

Currently I think this module uses the v1 api. The only breaking change is that v2 seperated the "started" from the "status"

{"status": "started", ...}
Management API v2 would report its status as:
{"status": "down", "started": true, ...}

Adding the slug field to the creation just requires adding it as an optional argument I think? I'm not sure how you want to handle the v2 change. I did a quit look earlier and didn't see anything referring to "started" so it would only be a breaking change for the actual playbooks that use the module, not within the module itself.

We could add a new parameter, e.g., api_version, which all modules require (perhaps defaulting to v3), and support the differences between all of the API versions this way.

@mamercad
Copy link
Collaborator

Please run black to get this test passing.

@Penagwin
Copy link
Author

We could add a new parameter, e.g., api_version, which all modules require (perhaps defaulting to v3), and support the differences between all of the API versions this way.

Yeah I can add that, I think we should default to v3, from what I could tell because of the way this module lists the returned object it's not actually affected by the v2 change.

We could make it so the create check fail if you specify the slug and it's on at least v3.

Technically you can also set the slug on updates which is only valid in v3. However this module does allow you to select the check by the slug, which would be valid in v2 if it's not changing? I'm not sure what the best way to validate that is.

@Penagwin
Copy link
Author

Do you want to have the api version just append the /v3 onto the base url?

I suppose we don't actually know what people will be mapping their full path to, it's possible they have just a http://localhost:8000/api type of thing.

@mamercad
Copy link
Collaborator

We could add a new parameter, e.g., api_version, which all modules require (perhaps defaulting to v3), and support the differences between all of the API versions this way.

Yeah I can add that, I think we should default to v3, from what I could tell because of the way this module lists the returned object it's not actually affected by the v2 change.

We could make it so the create check fail if you specify the slug and it's on at least v3.

Technically you can also set the slug on updates which is only valid in v3. However this module does allow you to select the check by the slug, which would be valid in v2 if it's not changing? I'm not sure what the best way to validate that is.

I think we're talking about supporting the, currently, three (3) versions of the API: v1, v2, and v3 (and defaulting to v3 if unspecified). In any event, more than likely, what we do with this will be a breaking changes for this collection (which isn't a big deal, we simply bump its major version).

There should be no need to "guess" at anything, if the end user specifies api_version: vN, they'll only have functionality which exists in API version N. If the user specifies api_version: v1 and tries to make use of functionality in a higher API version, it should fail with a helpful error message like X is not supported in v1, please set 'api_version: v2' or something along those lines.

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.

4 participants