-
Notifications
You must be signed in to change notification settings - Fork 55
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
fix: OpenAPI ModelVersion shall contain registeredModelId property #61
fix: OpenAPI ModelVersion shall contain registeredModelId property #61
Conversation
need to adapt property definition in OpenAPI to accomodate openapi-codegen result; according to contract (as also visible in swagger) the ModelVersion is to contain property: registeredModelId Signed-off-by: Matteo Mortari <[email protected]>
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.
LGTM, thanks @tarilabs
@@ -22,7 +22,7 @@ type OpenAPIConverter interface { | |||
// goverter:ignore Id CreateTimeSinceEpoch LastUpdateTimeSinceEpoch | |||
ConvertModelVersionCreate(source *openapi.ModelVersionCreate) (*openapi.ModelVersion, error) | |||
|
|||
// goverter:ignore Id CreateTimeSinceEpoch LastUpdateTimeSinceEpoch Name | |||
// goverter:ignore Id CreateTimeSinceEpoch LastUpdateTimeSinceEpoch Name RegisteredModelId |
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.
good catch 😃
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.
🙏
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.
Don't know exactly why we have these changes in this PR as nothing should have changed, but I tried locally as well and they are generated as is.
Moreover I don't even think they are called somewhere.
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.
I believe is something different than file scanning order, I never figured out as I guess it's something internal to the openapi-codegen 🤷
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.
yeah, the usual issue. Good for me 👍
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lampajr, rareddy, tarilabs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Py: Fix misleading errors on missing list results (#65) * py: fix type annotations to return concrete types Signed-off-by: Isabella Basso do Amaral <[email protected]> * py: fix missing type check on empty list results Signed-off-by: Isabella Basso do Amaral <[email protected]> --------- Signed-off-by: Isabella Basso do Amaral <[email protected]> * fix: OpenAPI ModelVersion shall contain registeredModelId property (#61) need to adapt property definition in OpenAPI to accomodate openapi-codegen result; according to contract (as also visible in swagger) the ModelVersion is to contain property: registeredModelId Signed-off-by: Matteo Mortari <[email protected]> --------- Signed-off-by: Isabella Basso do Amaral <[email protected]> Signed-off-by: Matteo Mortari <[email protected]> Co-authored-by: Isabella Basso <[email protected]> Co-authored-by: Matteo Mortari <[email protected]>
need to adapt property definition in OpenAPI
to accomodate openapi-codegen result;
according to contract (as also visible in swagger)
the
ModelVersion
is to contain property:registeredModelId
Description
How Has This Been Tested?
Merge criteria: