-
Notifications
You must be signed in to change notification settings - Fork 73
replace complete reference to model with trimmed identifier #324
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: mrmerlin320 <[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.
Hello @mrmerlin320, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request.
This PR aims to refine the representation of Model within the codebase and schemas. Specifically, it replaces the use of the full ModelDefinition struct with a more lightweight ModelIdentifier struct/schema in places where only identification is needed, rather than the complete model details. This change promotes better separation of concerns and potentially improves performance by avoiding unnecessary data loading.
Highlights
- New ModelIdentifier Type: A new Go struct
ModelIdentifierand its corresponding OpenAPI schema have been introduced to represent a trimmed-down version of a model, containing only essential identification fields (Name, Version, PlatformVersion). - Refactored Component Definition: The
Modelfield within theComponentDefinitionstruct in Go has been updated to use the newModelIdentifiertype instead of the fullModelDefinition. - Schema Simplification: JSON and OpenAPI schemas for
ComponentandRelationshiphave been updated to reference the newModelIdentifierschema, removing verbose inline definitions of the full model where only the identifier is required. - Clarified Versioning: Comments and schema descriptions related to version fields in the
model.gofile and associated schemas have been updated to clearly distinguish between the 'Version of the model definition' and the 'Version of the platform for which the model is intended'.
Changelog
Click here to see the changelog
- models/v1beta1/component/component.go
- Changed the type of the
Modelfield inComponentDefinitionfrommodel.ModelDefinitiontomodel.ModelIdentifier(Line 127). - Updated the comment for the
Modelfield to reflect it's now an identifier (Line 126).
- Changed the type of the
- models/v1beta1/model/model.go
- Added a type alias
PlatformVersionfor string (Line 65). - Updated the comment for the
Versionfield in theModelstruct to clarify it's the platform version (Line 157). - Updated the comment for the
Versionfield in theModelDefinitionstruct to clarify it's the model definition version (Line 169). - Updated the comment for the nested
Versionfield within theModelstruct inModelDefinitionto clarify it's the platform version (Line 208). - Added a new struct
ModelIdentifierwithPlatformVersion,Name, andVersionfields (Lines 260-268).
- Added a type alias
- schemas/constructs/v1alpha3/relationship.json
- Replaced the inline definition of the 'model' property with a
$refto the newModelIdentifierschema (Lines 59-63).
- Replaced the inline definition of the 'model' property with a
- schemas/constructs/v1beta1/component/component.json
- Updated the
$reffor the 'model' property from../model/model.jsonto../model/openapi.yml#/components/schemas/ModelIdentifier(Line 68). - Changed the
x-go-typefor the 'model' property tomodel.ModelIdentifier(Line 69). - Removed the detailed description for the 'model' property (Line 74).
- Updated the
- schemas/constructs/v1beta1/component/merged-openapi.yml
- Updated the
x-go-typefor the 'model' property tomodel.ModelIdentifier(Line 98). - Removed the detailed inline properties for the 'model' property (Lines 113-924).
- Added a
$refto theModelIdentifierschema definition (Line 106). - Added
descriptionandrequiredfields for the 'model' property (Lines 107-111).
- Updated the
- schemas/constructs/v1beta1/model/merged-openapi.yml
- Added the
ModelIdentifierschema definition (Lines 179-218). - Added the
PlatformVersionschema definition (Lines 220-231). - Updated the
versionproperty within theModelschema to referencePlatformVersion(Line 241). - Added
definitionsfornameandversion(Lines 261-279). - Updated the
versionproperty withinModelDefinitionto reference the newversiondefinition (Line 318). - Updated the
nameproperty withinModelDefinitionto reference the newnamedefinition (Line 337). - Updated the
versionproperty within the nestedModelstruct inModelDefinitionto referencePlatformVersion(Line 1037).
- Added the
- schemas/constructs/v1beta1/model/model.json
- Added
definitionssection fornameandversion(Lines 8-26). - Updated the
versionproperty to use$refto the newversiondefinition (Line 50). - Updated the
nameproperty to use$refto the newnamedefinition (Line 58).
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A model, full and grand,
Was trimmed by skillful hand.
An identifier, neat and small,
Now answers duty's call.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request replaces complete model references with trimmed identifiers, enhancing maintainability and data efficiency. The introduction of PlatformVersion clarifies versioning. However, there are critical issues with JSON/YAML serialization tags that need to be addressed.
Summary of Findings
- JSON/YAML Tag Collision in ModelIdentifier: The
PlatformVersionandVersionfields in theModelIdentifierGo struct are both tagged asjson:"version"andyaml:"version". This is a critical issue causing serialization/deserialization conflicts. The tags forPlatformVersionneed to be changed to be unique (e.g.,platform_versionorplatformVersion). - OpenAPI Schema Inconsistency for ModelIdentifier: The OpenAPI schemas defining or using
ModelIdentifier(e.g., inmodel/merged-openapi.ymlandcomponent/merged-openapi.yml) have a field (often namedplatform_versionin YAML) that is incorrectly tagged to producejson:"version". This conflicts with the model definition'sversionfield and must be updated to align with the corrected Go struct tags, using a unique JSON key likeplatform_versionorplatformVersion.
Merge Readiness
This pull request is not ready to be merged due to a critical issue with JSON/YAML tag collisions in the ModelIdentifier struct and related schema inconsistencies. These issues must be addressed to ensure correct serialization and deserialization. Once these are resolved, the PR will be in much better shape. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from team members before merging.
| "model": { | ||
| "$ref": "../model/model.json", | ||
| "x-go-type": "model.ModelDefinition", | ||
| "$ref": "../model/openapi.yml#/components/schemas/ModelIdentifier", |
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.
Hey hello 😇 !
I think we should aim to have reference to json schema as much as we can and not to openapi.
Maybe this lays good on frontend, but f.e. backend struggle to handle this.
is it possible to achieve same result with referencing json schema definition, inside model/model.json or in separate schema file. ?
Notes for Reviewers
This PR fixes #
Signed commits