-
Notifications
You must be signed in to change notification settings - Fork 65
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 <mrmerlin320@gmail.com>
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
ModelIdentifier
and 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
Model
field within theComponentDefinition
struct in Go has been updated to use the newModelIdentifier
type instead of the fullModelDefinition
. - Schema Simplification: JSON and OpenAPI schemas for
Component
andRelationship
have been updated to reference the newModelIdentifier
schema, 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.go
file 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
Model
field inComponentDefinition
frommodel.ModelDefinition
tomodel.ModelIdentifier
(Line 127). - Updated the comment for the
Model
field to reflect it's now an identifier (Line 126).
- Changed the type of the
- models/v1beta1/model/model.go
- Added a type alias
PlatformVersion
for string (Line 65). - Updated the comment for the
Version
field in theModel
struct to clarify it's the platform version (Line 157). - Updated the comment for the
Version
field in theModelDefinition
struct to clarify it's the model definition version (Line 169). - Updated the comment for the nested
Version
field within theModel
struct inModelDefinition
to clarify it's the platform version (Line 208). - Added a new struct
ModelIdentifier
withPlatformVersion
,Name
, andVersion
fields (Lines 260-268).
- Added a type alias
- schemas/constructs/v1alpha3/relationship.json
- Replaced the inline definition of the 'model' property with a
$ref
to the newModelIdentifier
schema (Lines 59-63).
- Replaced the inline definition of the 'model' property with a
- schemas/constructs/v1beta1/component/component.json
- Updated the
$ref
for the 'model' property from../model/model.json
to../model/openapi.yml#/components/schemas/ModelIdentifier
(Line 68). - Changed the
x-go-type
for 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-type
for the 'model' property tomodel.ModelIdentifier
(Line 98). - Removed the detailed inline properties for the 'model' property (Lines 113-924).
- Added a
$ref
to theModelIdentifier
schema definition (Line 106). - Added
description
andrequired
fields for the 'model' property (Lines 107-111).
- Updated the
- schemas/constructs/v1beta1/model/merged-openapi.yml
- Added the
ModelIdentifier
schema definition (Lines 179-218). - Added the
PlatformVersion
schema definition (Lines 220-231). - Updated the
version
property within theModel
schema to referencePlatformVersion
(Line 241). - Added
definitions
forname
andversion
(Lines 261-279). - Updated the
version
property withinModelDefinition
to reference the newversion
definition (Line 318). - Updated the
name
property withinModelDefinition
to reference the newname
definition (Line 337). - Updated the
version
property within the nestedModel
struct inModelDefinition
to referencePlatformVersion
(Line 1037).
- Added the
- schemas/constructs/v1beta1/model/model.json
- Added
definitions
section forname
andversion
(Lines 8-26). - Updated the
version
property to use$ref
to the newversion
definition (Line 50). - Updated the
name
property to use$ref
to the newname
definition (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
PlatformVersion
andVersion
fields in theModelIdentifier
Go struct are both tagged asjson:"version"
andyaml:"version"
. This is a critical issue causing serialization/deserialization conflicts. The tags forPlatformVersion
need to be changed to be unique (e.g.,platform_version
orplatformVersion
). - OpenAPI Schema Inconsistency for ModelIdentifier: The OpenAPI schemas defining or using
ModelIdentifier
(e.g., inmodel/merged-openapi.yml
andcomponent/merged-openapi.yml
) have a field (often namedplatform_version
in YAML) that is incorrectly tagged to producejson:"version"
. This conflicts with the model definition'sversion
field and must be updated to align with the corrected Go struct tags, using a unique JSON key likeplatform_version
orplatformVersion
.
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.
@@ -65,13 +65,12 @@ | |||
} | |||
}, | |||
"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