-
Notifications
You must be signed in to change notification settings - Fork 11
chore: Update OpenAPI Spec for SDK #331
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
=======================================
Coverage 83.76% 83.76%
=======================================
Files 56 56
Lines 3961 3961
=======================================
Hits 3318 3318
Misses 643 643 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1,3 +1,5 @@ | |||
| post_hooks: | |||
| # Fix missing status codes in generated API files | |||
| - 'python ../../../scripts/patch_status_codes.py "../../../src/galileo/resources/api"' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vamaq Added this new script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check if openapi modified their codegen_templates/endpoint_module.py.jinja file and if we need to pull those changes into our version and re-apply our modifications to that file back on top :)
@jrhone Quick FYI the updates dont work and are still breaking for me |
|
|
||
| headers["Content-Type"] = "application/json" | ||
|
|
||
| headers["X-Galileo-SDK"] = f"galileo-python/{get_package_version()}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add this manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see it's in the jinja template.. interesting that it was missing from this file and not the rest, I guess doesn't matter 🤷🏾♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But actually yeah I haven't checked every file.. github is forcing me to look at each file one at a time 😂
| ) -> Optional[Union[BulkDeleteDatasetsResponse, HTTPValidationError]]: | ||
| if response.status_code == 200: | ||
| response_200 = BulkDeleteDatasetsResponse.from_dict(response.json()) | ||
| return BulkDeleteDatasetsResponse.from_dict(response.json()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it generates with if response.status_code == :
Then the script runs to convert to if response.status_code == 200:
Then the linter makes the change from response_200 = to return?
Gotcha... I'm not sure about using a script to fix what should be built-in functionality. I mean we're not customizing the functionality.. the tool is just breaking. I suppose if no other fix then happy to approve, but, what are your thoughts on that? |
|
Wasn't possible to separate linter changes and actual changes in different PRs? If it's a headache don't worry, I pushed the diff through GPT and looks clean. |
|
Should we see if an updated version of Seems we're on 0.25.3 In version 0.26.0 there was a commit related to http status codes which updated the templates I thought we had auto upgraded to that version which caused the break but that makes sense now why trying the latest templates didn't fix it for you.. but if we pull in the latest version maybe it will.. maybe not. |
jrhone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I guess my only real comment above is what happens if we update our openapi-python-client to the latest.. maybe it will fix the status code issue since there were changes directly related to status codes in the new version.
Ideally we wouldn't fix breaking functionality with a patch.. but otherwise looks good.
|
Oh, and looks like a new version was deployed to API recently that has changes I need for my other tasks.. if you wouldn't mind re-generating with that version since the re-generation breaks for me. Or feel free to merge this PR in as is but I might bug you to regenerate a fresh one 🙏🏾 |
No description provided.