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

Initial Swagger integration #512

Merged
merged 7 commits into from
May 11, 2022
Merged

Initial Swagger integration #512

merged 7 commits into from
May 11, 2022

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented May 10, 2022

This PR is the inception for exposing our APIs version 1.0.0.

This very first iteration aims to:

As of now only the Hosts List API is documented.

Here's how that looks like (nothing new 😄)

vokoscreenNG-2022-05-10_17-49-15.mp4

Help me please with defining what should be exposed and what not.
Here's a quick idea.

Expose

  • GET /api/hosts
  • GET /api/clusters
  • GET /api/sap_systems
  • GET /api/databases
  • POST /api/clusters/:cluster_id/checks
  • POST /api/clusters/:cluster_id/checks/request_execution
  • GET /api/checks/catalog
  • POST /api/runner/callback

Not sure

  • POST /api/collect
  • GET /api/installation/api-key
  • GET /api/sap_systems/health
  • GET /api/clusters/:cluster_id/connection_settings
  • PUT /api/clusters/:cluster_id/connection_settings
  • POST /api/hosts/:id/tags
  • DELETE /api/hosts/:id/tags/:value
  • POST /api/clusters/:id/tags
  • DELETE /api/clusters/:id/tags/:value
  • POST /api/sap_systems/:id/tags
  • DELETE /api/sap_systems/:id/tags/:value
  • POST /api/databases/:id/tags
  • DELETE /api/databases/:id/tags/:value

No Doc

  • GET /api/about
  • GET /api/settings
  • POST /api/accept_eula
  • POST /api/hosts/:id/heartbeat
  • GET /api/prometheus/targets

Extra considerations:
at the moment we're adding schemas only, later we'll add also DTOs on top of readmodels to decouple layers.
We can start by having those DTOs and respective OpenApi schemas next to each other (maybe in the same file) and since we're still not sure how much of a burden it will become to maintain, when and if needed we might want to add some macro magic like def_openapi_schema (or any better name) to link indeed DTOs and schemas.

Again this is a first iteration, let's see how that evolves. ✌️

@nelsonkopliku nelsonkopliku force-pushed the swagger_integration branch 4 times, most recently from 9d1e24d to f5ad760 Compare May 11, 2022 08:20
@nelsonkopliku nelsonkopliku marked this pull request as ready for review May 11, 2022 08:25
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hey @nelsonkopliku ,

The functionality is exactly as expected. Perfect!

As you comment, the need of defining all the fields back again here looks pretty annoying. It looks like something where we easily could make mistakes. And again, as you comment, having some sort of logic which takes the data models from the code to create the spec would be the ideal scenario. I don't know having the DTOs would directly enable this, but it looks something interesting to explore.

From me side, we can merge this as initial iteration, and continue exploring the option to "auto" generate the models in next iterations.

provider_data: TrentoWeb.OpenApi.Schema.Provider.ProviderData,
tags: %Schema{
title: "Tags",
description: "A list of tags attached to a resource",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can be more specific here A list of tags attached to a host.
Not big deal anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

We tag several resources next to hosts: clusters, sap systems, databases.
This same schema could be used for those scenarios.

Does it make sense?

lib/trento_web/openapi/schema/host.ex Outdated Show resolved Hide resolved
lib/trento_web/openapi/schema/host.ex Outdated Show resolved Hide resolved
@fabriziosestito
Copy link
Member

fabriziosestito commented May 11, 2022

@arbulu89 |

As you comment, the need of defining all the fields back again here looks pretty annoying. It looks like something where we easily could make mistakes.

I understand the concern but keep in mind this is done once for API version for documentation.
Once we go public with a v1 we will not change the API contract (expect mostly additions) till v2, so I don't think this will be a problem.
A v2 will happen only if we make breaking changes to the API, e.g. in a non-backwards-compatible way.

@nelsonkopliku nelsonkopliku force-pushed the swagger_integration branch from 968fbfa to 9cae091 Compare May 11, 2022 12:38
@nelsonkopliku nelsonkopliku force-pushed the swagger_integration branch from 9cae091 to 0f90223 Compare May 11, 2022 12:41
@nelsonkopliku nelsonkopliku force-pushed the swagger_integration branch from 0f90223 to deb9da3 Compare May 11, 2022 12:59
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

I think we are ready to merge

Copy link
Member

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

LGTM! sorry forgot to approve as always

@nelsonkopliku nelsonkopliku merged commit 44fc08e into main May 11, 2022
@nelsonkopliku nelsonkopliku deleted the swagger_integration branch May 11, 2022 13:17
@nelsonkopliku nelsonkopliku added documentation Improvements or additions to documentation enhancement New feature or request labels May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants