-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update OpenAPI to v3.1 #854
Conversation
9a7bf74
to
4cf1d2b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
=======================================
Coverage 88.57% 88.57%
=======================================
Files 129 129
Lines 1725 1725
Branches 213 220 +7
=======================================
Hits 1528 1528
+ Misses 197 182 -15
- Partials 0 15 +15 ☔ View full report in Codecov by Sentry. |
I ran the sdk generator locally and it doesn't look like it's up to snuff. For instance: ApplicationForm.ts (main branch) import {
ApplicationFormField,
} from './ApplicationFormField';
import { Writable } from './Writable';
export interface ApplicationForm {
readonly id: number;
opportunityId: number;
readonly version: number;
fields?: Array<ApplicationFormField>;
readonly createdAt: Date;
}
export type WritableApplicationForm = Writable<ApplicationForm> ApplicationForm.ts (generated from this branch) import { Writable } from './Writable';
export interface ApplicationForm {
readonly id: any;
opportunityId: any;
readonly version: any;
fields?: any;
readonly createdAt: any;
}
export type WritableApplicationForm = Writable<ApplicationForm> I'll look into whether or not this is a swagger-codegen limitation or if I'm doing something wrong here. |
Based on swagger-api/swagger-codegen#12210 it looks like codegen doesn't support 3.1.0 yet -- it isn't clear if / when it ever will, though it does seem that the core technologies have been updated to support it, so maybe there's just a dependency version that needs updating somewhere. |
Once this PR is merged we should be able to finish this one: PhilanthropyDataCommons/sdk#26 |
4cf1d2b
to
d6e2d75
Compare
You mean the opposite, right? |
From the PR description:
Is this outdated? I don't see anything referencing nullability. |
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.
In combination with PhilanthropyDataCommons/sdk#26, this worked locally (with all the same caveats re: front end needing to adapt to some changes).
@reefdog whoops I accidentally nuked the nullable change, I'll either confirm it's needed and add that back in, or will update the PR description. If I add the changes I may ping you for another review, if I just edit the PR description I will not! That said, we do want to merge the SDK generator changes first since this PR is what will trigger the build / publication of the sdk (and if we merge this before updating the generator, the generator will create a very broken SDK). EDIT: I'm now seeing the problem -- I wrote "once this pr is merged we'll be able to finish this one" and there is absolutely no way of knowing which "this" refers to THIS pr vs the linked PR. Good job Dan. |
@slifty 😆 Yes, that was the source of my confusion. Pronoun antecedents! |
We paused the project around May 25th so this got stale! Reviewing everything and will nudge this to the next step so we can benefit from 3.1 soon |
Swagger supports OpenAPI v3.1.0 [1] so we should use it. Issue #849 Update to OpenAPI 3.1 [1]https://swagger.io/blog/swagger-supports-openapi-3-1/
d6e2d75
to
ded7b33
Compare
This PR updates our OpenAPI spec to v3.1
It also makes a small change to remove nullability from a few fields, which should no longer be nullable. This was done as part of the migration because it appears v3.1 changes the way the
nullable
keyword works.Resolves #849
This should NOT be merged until after: