Skip to content

Conversation

@DO-rrao
Copy link
Collaborator

@DO-rrao DO-rrao commented Nov 3, 2025

context

Slack thread
GET and CREATE have the different responses and we are using the same structure for both. This is causing mismatch for GET.

code change

created new response without wrapper sink obj and used it for the GET response.

response screenshot

Screenshot 2025-11-03 at 1 05 41 PM

testing

  • Tested all 5 Database Logsink API endpoints (CREATE, GET, LIST, UPDATE, DELETE) using MySQL cluster
  • Verified correct HTTP status codes: CREATE returns 201, GET/LIST return 200, UPDATE/DELETE return 204.

openapi screenshot

Uploading Screenshot 2025-11-03 at 1.06.36 PM.png…

@DO-rrao DO-rrao requested a review from anup-deka November 3, 2025 07:37
@do-joe
Copy link

do-joe commented Nov 3, 2025

EDITED to reflect logsinks.yml as well

One suggestion for reducing duplication. We now have three files that independently reference the same logsink schema:

  • logsink.yml (CREATE - wrapped): {"sink": {...}}
  • logsink_data.yml (GET - unwrapped): {...}
  • logsinks.yml (LIST - wrapped array): {"sinks": [...]}

Each independently defines the same allOf reference, required fields, and examples. This means any future changes to the logsink structure need updates in three places.

Could we refactor to have a single shared schema definition that all three responses reference? For example (not 100% I got the syntax correct, but to illustrate the point)

Create a base schema file (logsink_schema.yml):

allOf:
  - $ref: "../models/logsink_verbose.yml"
required:
  - sink_id
  - sink_name
  - sink_type
  - config

Then each response file just handles wrapping/structure:

  • logsink_data.yml references it directly (unwrapped)
  • logsink.yml wraps it with sink property
  • logsinks.yml wraps it as an array in sinks

This would centralize the schema definition and make future maintenance much easier.

Copy link
Contributor

@anup-deka anup-deka left a comment

Choose a reason for hiding this comment

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

LGTM

@do-joe
Copy link

do-joe commented Nov 4, 2025

LGTM, thanks!

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