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

Added healthcheck endpoint #173

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

sheldor1510
Copy link
Contributor

@larsks For #160 , I discussed with @tzumainn and decided to do the healthcheck at the root endpoint (<host_url>/) similar to how the Ironic API does.

So, the API returns this at <host_url>/:

{
  "name": "ESI Leap API",
  "description": "ESI Leap is an OpenStack service for leasing baremetal nodes, designed to run on top of multi-tenant Ironic.",
  "versions": [
    {
      "id": "v1.0",
      "status": "CURRENT",
      "links": [
        {
          "href": "<host_url>/v1",
          "rel": "self"
        }
      ]
    }
  ]
}


The rest of the requests return this if accessed unauthenticated:

{
  "error": {
    "code": 401,
    "title": "Unauthorized",
    "message": "The request you have made requires authentication."
  }
}

Copy link
Member

@larsks larsks left a comment

Choose a reason for hiding this comment

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

@tzumainn I worry that we are conflating two things here:

  1. Health check
  2. Version discovery

If we're going to report a versions string, then either (a) there should be code to dynamically discover what API versions are supported by the code or (b) there should be a way to register version information.

What do you think?

@tzumainn
Copy link
Contributor

@tzumainn I worry that we are conflating two things here:

  1. Health check
  2. Version discovery

If we're going to report a versions string, then either (a) there should be code to dynamically discover what API versions are supported by the code or (b) there should be a way to register version information.

What do you think?

To be honest I don't see a huge practical difference between the two (although I could be missing something)! I do agree with your comment about versions though.

@larsks
Copy link
Member

larsks commented Aug 2, 2024

To be honest I don't see a huge practical difference between the two

The difference is that the health check endpoint doesn't really require any additional logic, so it can be implemented immediately, while setting up version discovery seems like a bigger project.

I defer to your judgement on this; if you're happy with the current implementation, I'm not going to object.

@tzumainn

@sheldor1510
Copy link
Contributor Author

sheldor1510 commented Aug 2, 2024

@larsks I had discussed with @tzumainn and planned to implement the API versioning functionality (which will help us with version discovery for the returned object) in this PR. Should I go ahead with that on this PR or make a separate PR?

@larsks
Copy link
Member

larsks commented Aug 2, 2024

@sheldor1510 I would make that a separate PR.

from oslo_serialization import jsonutils


def root(host_url):
Copy link
Contributor

Choose a reason for hiding this comment

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

may as well create a unit test for this, just for completeness!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created and tested!

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

@sheldor1510 sheldor1510 requested a review from tzumainn August 6, 2024 20:50
Copy link
Contributor

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

Looks good! One last request - can you squash your commits into a single commit? That helps maintain a clean commit history.

added unit tests

pre-commit fix
@sheldor1510 sheldor1510 force-pushed the healthcheck-endpoint branch from 48a6c55 to 24d2b83 Compare August 7, 2024 16:27
@sheldor1510 sheldor1510 requested a review from tzumainn August 7, 2024 16:48
Copy link
Contributor

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

looks good - thanks!

@tzumainn tzumainn merged commit a2bad03 into CCI-MOC:master Aug 7, 2024
7 checks passed
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.

3 participants