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

✨ [#1617] added feature to see if connection is alive in ser… #91

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

bart-maykin
Copy link
Contributor

@bart-maykin bart-maykin commented May 14, 2024

…vice admin list page

Fixes #1617
Fixes some aspects of #12

Added health_check read_only_field in the service admin panel

@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch 2 times, most recently from 7b27e39 to 4365497 Compare May 16, 2024 06:12
@bart-maykin bart-maykin requested a review from annashamray May 16, 2024 06:14
@bart-maykin bart-maykin marked this pull request as ready for review May 16, 2024 06:18
@annashamray annashamray requested review from SonnyBA, stevenbal and sergei-maertens and removed request for annashamray May 16, 2024 08:41
@sergei-maertens
Copy link
Member

I'll review this tomorrow

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

First of all - this implementation is not according to the specification in the ticket:

I'd like to have a 'Check configuration' button in the Service detailview that performs a GET request to the API root URL with the authentication-options configured and shows the response.

The way this PR implements it, causes this check request to be made every time you open the detail view/edit form of a service (and I'm also not sure how this behaves when you are adding new objects rather than editing an existing one).

This is the opposite of performing an explicit check after clicking a button. So please change the UX/UI by displaying a button or link to test the connection, which takes you to a custom admin view. In this admin view, I expect a form with the following fields:

  • Method (GET / POST), default to GET
  • Endpoint to test (forms.CharField). It's okay to add a model field health_check_path like you did. This path needs to be appended to the api_root, so it cannot be absolute.

A button to submit the form, and on submissions, the health check is performed and the result reported back.

The reason for this is:

  • performance - you don't want to spam the remote service every time you are editing the configuration
  • a service may not be reachable with GET methods only, e.g. a SOAP endpoint typically only allows POST requests
  • Diagnosing a service may require the user to try different endpoints, so having a health check path as default is fine, but it should be possible to override this at check-time

An finally, model tests alone are not sufficient. You are modifying admin behaviour, so this needs client/view tests too to ensure nothing has broken and the feature is working as intended.

@joeribekker
Copy link
Member

Refinement: Bart and I discussed and the way its implemented matches what we discussed (but I didnt wrote it down as such afterwards). All technical issue can be addressed and or answered @bart-maykin.

We can rename it to "connection check" or whatever. This is an admin perspective on whether we can connect to an endpoint and not so much a healthcheck (but named so because Bart thought Sergei wanted this).

@joeribekker
Copy link
Member

Return status code please instead of boolean.

Copy link

@SonnyBA SonnyBA left a comment

Choose a reason for hiding this comment

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

One question regarding performing the actual request but overall looks good to me 👍

@bart-maykin bart-maykin requested review from stevenbal and SonnyBA June 11, 2024 08:50
@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch from d29bb07 to 381ae54 Compare June 19, 2024 05:53
@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch 2 times, most recently from 41c87d9 to 821df82 Compare June 19, 2024 06:11
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

nitpicking 😬

@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch from 821df82 to 3e30c39 Compare July 25, 2024 12:38
@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch from 3e30c39 to ce07e64 Compare July 25, 2024 13:09
@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch from ce07e64 to dcf8682 Compare July 25, 2024 13:42
@bart-maykin bart-maykin force-pushed the feature/1617-service-health-check branch from dcf8682 to 923b5f6 Compare July 25, 2024 13:48
@bart-maykin bart-maykin merged commit 0f5fe38 into main Jul 25, 2024
7 checks passed
@bart-maykin bart-maykin deleted the feature/1617-service-health-check branch July 25, 2024 13:55
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.

Improve Services / zgw-consumers to allow checking the Service configuration when testing
5 participants