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

chore: support "formData" in swagger bindings parsing #8078

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

tayritenour
Copy link
Contributor

@tayritenour tayritenour commented Oct 4, 2023

Description

We are using the swagger generation in lore, and need to support openapi.json that describe endpoints with formData as well. This doesn't change anything about the current bindings (which I ran with make -C proto build && make -C bindings build && make -C master build && make -C proto gen-buf-image)

This is a workaround due to coupling with Lore. A proper fix ticket is here: https://hpe-aiatscale.atlassian.net/browse/MLG-1019

Test Plan

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

@netlify
Copy link

netlify bot commented Oct 4, 2023

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 8ec6d50
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/651f175b18759a0008e2244a
😎 Deploy Preview https://deploy-preview-8078--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tayritenour tayritenour enabled auto-merge (squash) October 4, 2023 21:45
@szewaiyuen6
Copy link
Contributor

szewaiyuen6 commented Oct 4, 2023

What happen if we run this generation code on Lore, will there be no bindings created for the upload end point? Or the bindings would still be created but we just don't use it?

@tayritenour
Copy link
Contributor Author

tayritenour commented Oct 5, 2023

What happen if we run this generation code on Lore, will there be no bindings created for the upload end point? Or the bindings would still be created but we just don't use it?

When we run on Lore, it makes the bindings for all the objects and endpoints successfully. Before it was completely blocked from working so we need this allowance to do the build at all. The issue is that the generated endpoint for specifically anything that uses formData will not pass the files correctly. We need to do the supplemental work to actually support Multipart form data to use it. Ticket here: https://hpe-aiatscale.atlassian.net/browse/MLG-1019

In the mean time, caleb is okay with just calling the particular upload_files endpoint directly (rather than using the bindings). The reason being that doing this correctly looks complicated and would take me a while and I don't know if we can request the resources to do this quickly enough for the demo.

Copy link
Contributor

@szewaiyuen6 szewaiyuen6 left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks for the explanation.

@tayritenour tayritenour enabled auto-merge (squash) October 5, 2023 17:29
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

looks fine!

@tayritenour tayritenour merged commit ecc0f6b into main Oct 5, 2023
68 of 83 checks passed
@tayritenour tayritenour deleted the enable_swagger_parse_form_data branch October 5, 2023 20:30
@dannysauer dannysauer added this to the 0.26.2 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants