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: fix ListValue types in swagger spec #4801

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

rb-determined-ai
Copy link
Member

Protobuf google.protobuf.ListValue types come through swagger as:

{"type":"array", "items":{"type":"object"} }

which would convert to Sequence[Dict[str, Any]], which is a pretty
strange field type for v1PatchModel.labels.

This PR adds tweaks via the proto/patches/api.json to force the right
type in the swagger.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit ac48f70
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/62fd2189db068f00088a122f
😎 Deploy Preview https://deploy-preview-4801--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 settings.

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

I'm worried about what happens when the endpoint or the proto definitions change :thinking_face: would people realize that their changes didn’t result in generating the change they expected?

update: the JSON patch seems to so precisely target the type of the label that I’m not worried

hbp ➜ determined git:(master) jq '.definitions.v1PatchExperiment' proto/build/swagger/determined/api/v1/api.swagger.json > /tmp/before.json
hbp ➜ determined git:(master) colordiff /tmp/before.json /tmp/after.json
16c16
<         "type": "object"
---
>         "type": "string"
hbp ➜ determined git:(master)

looking at the rest of changes

@hamidzr hamidzr self-requested a review August 17, 2022 16:56
@rb-determined-ai rb-determined-ai force-pushed the list-value branch 2 times, most recently from 3910dc5 to 0db5a53 Compare August 17, 2022 16:59
Protobuf google.protobuf.ListValue types come through swagger as:

    {"type":"array", "items":{"type":"object"} }

which would convert to `Sequence[Dict[str, Any]]`, which is a pretty
strange field type for v1PatchModel.labels.

This PR adds tweaks via the proto/patches/api.json to force the right
type in the swagger.
Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

lgtm

@hamidzr hamidzr assigned rb-determined-ai and unassigned hamidzr Aug 17, 2022
@rb-determined-ai rb-determined-ai merged commit 69f5dfb into determined-ai:master Aug 17, 2022
@rb-determined-ai rb-determined-ai deleted the list-value branch August 17, 2022 21:02
NicholasBlaskey pushed a commit to NicholasBlaskey/determined that referenced this pull request Aug 24, 2022
Protobuf google.protobuf.ListValue types come through swagger as:

    {"type":"array", "items":{"type":"object"} }

which would convert to `Sequence[Dict[str, Any]]`, which is a pretty
strange field type for v1PatchModel.labels.

This PR adds tweaks via the proto/patches/api.json to force the right
type in the swagger.
@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.2 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.

3 participants