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

feat(api): respond with 200/201 with empty json for successful PUT #12642

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

slonka
Copy link
Contributor

@slonka slonka commented Jan 22, 2025

Motivation

In our OpenAPI spec we say that we're returning application/json for put 200/201

and we don't always do that. I can't find a way to describe this with OpenAPI schema (either you don't set content or you set it, there is no oneOf on that level). I tried this:

      responses:
        '200':
          description: Updated
          content:
            application/json:
              schema:
                oneOf:
                  - $ref: '#/components/schemas/MeshCreateOrUpdateSuccessResponse'
                  - type: 'null'

If we don't do this change terraform does not like the response not having content type:

╷
│ Error: failure to invoke API
│
│   with konnect_mesh.default,
│   on main.tf line 24, in resource "konnect_mesh" "default":
│   24: resource "konnect_mesh" "default" {
│
│ unknown content-type received: : Status 200
╵

Implementation information

Respond with 200/201 and empty {} when there are no errors.

Supporting documentation

xrel https://github.com/Kong/kong-mesh/issues/7201

Copy link
Contributor

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

  • Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
  • PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
  • IPv6 is taken into account (.e.g: no string concatenation of host port)
  • Tests (Unit test, E2E tests, manual test on universal and k8s)
    • Don't forget ci/ labels to run additional/fewer tests
  • Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

@slonka slonka force-pushed the add-content-type-on-put branch from 2b7506e to 05553eb Compare January 22, 2025 19:26
@slonka slonka changed the title feat(api): set content type on response without warnings feat(api): respond with 204 on empty responses for PUT Jan 22, 2025
@slonka slonka force-pushed the add-content-type-on-put branch from 05553eb to 6a43880 Compare January 23, 2025 07:35
@slonka slonka force-pushed the add-content-type-on-put branch from 6a43880 to eb53d57 Compare January 23, 2025 07:48
@slonka slonka marked this pull request as ready for review January 23, 2025 08:02
@slonka slonka requested a review from a team as a code owner January 23, 2025 08:02
@lahabana
Copy link
Contributor

@slonka title of the PR needs updating

@slonka slonka changed the title feat(api): respond with 204 on empty responses for PUT feat(api): respond with 200/201 with empty json for PUT Jan 23, 2025
@slonka slonka force-pushed the add-content-type-on-put branch from 6643bfb to 44a8b82 Compare January 23, 2025 08:56
@slonka slonka requested review from lobkovilya and lahabana January 23, 2025 08:58
@slonka slonka changed the title feat(api): respond with 200/201 with empty json for PUT feat(api): respond with 200/201 with empty json for successful PUT Jan 23, 2025
@slonka slonka merged commit b7131bb into kumahq:master Jan 23, 2025
14 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.

4 participants