-
Notifications
You must be signed in to change notification settings - Fork 160
feat: Add ConnectionDefinition support to registration utilities #778
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?
feat: Add ConnectionDefinition support to registration utilities #778
Conversation
- Add connections.meshery.io schema version support in FindEntityType - Make getEntity function public as GetEntity for testing - Update PackagingUnit to include ConnectionDefinition slice - Add ConnectionDefinition handling in processDir function - Extend register function to process and register connections - Add ConnectionDefinition to RegistryManager AutoMigrate - Fix import aliases and type references in registry.go This enables the model import pipeline to correctly process and register ConnectionDefinition entities alongside components and relationships. Signed-off-by: HeerakKashyap <[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.
Summary of Changes
Hello @HeerakKashyap, 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!
This pull request addresses a critical gap in the MeshModel registry by implementing comprehensive support for ConnectionDefinition
entities. It integrates these new entities into the existing registration pipeline, from file processing and parsing to database persistence, ensuring that the registry can now fully manage all defined MeshModel entity types. Additionally, it introduces several utility functions to enhance the robustness of file and I/O operations across the codebase.
Highlights
- Full Support for Connection Definitions: This pull request enables the Meshery MeshModel registry to correctly identify, parse, and register
ConnectionDefinition
entities, which were previously ignored during model import. This completes the support for all MeshModel entity types (components, relationships, and connections). - Enhanced Entity Parsing and Identification: The utility functions responsible for determining entity types and parsing YAML/JSON content have been updated to recognize
ConnectionDefinition
schemas, ensuring they are correctly processed within the system. - Improved File Handling Robustness: Several file and I/O operations across the codebase have been made more robust by introducing
SafeClose
andSafeRemoveAll
helper functions, preventing potential resource leaks or unhandled errors and promoting safer resource management. - Database Schema Update: The underlying database schema has been updated to include
ConnectionDefinition
entities, allowing them to be persisted and managed within the MeshModel registry.
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 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 or fill out our survey 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.
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 successfully adds support for ConnectionDefinition
entities to the model registration process. The changes are logical and cover database migration, entity discovery, and registration logic.
I've identified a few areas for improvement:
- A high-severity issue regarding the use of a loop variable's address, which is a dangerous pattern in Go.
- Several medium-severity issues related to code clarity and maintainability, such as redundant code in an error path, use of hardcoded strings instead of constants, a redundant comment, and a potentially unintentional change that reduces the clarity of truncated error messages.
Applying these suggestions will improve the robustness and maintainability of the code.
models/registration/dir.go
Outdated
conn, err := utils.Cast[*corev1beta1.ConnectionDefinition](e) | ||
if err != nil { | ||
connectionName := "" | ||
if conn != nil { | ||
connectionName = conn.Kind | ||
} |
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.
Thank you for your contribution!! |
Yeah sure, I guess there are some small errors which needs to be solved, if fixed timely - I'll try to present it in development meeting, thankss @vr-varad |
Thank you @HeerakKashyap for your contribution!! |
Yeah I'll let u know as soon as I am done with solving the bugs |
9fee88d
to
3f08e48
Compare
…x code review issues - Add missing ConnectionDefinition entity type constant - Create ConnectionDefinition type in core v1beta1 package - Fix hardcoded string by adding ConnectionSchemaVersion constant - Fix loop variable issue in connection registration - Remove redundant nil check in connection file handling - Ensure all code compiles successfully Signed-off-by: HeerakKashyap <[email protected]>
3f08e48
to
2b2461c
Compare
- Fix import order to follow Go conventions - Improve error handling in GenerateID method - Properly handle errors in Create method instead of ignoring them Signed-off-by: HeerakKashyap <[email protected]>
- Replace deprecated oras.PackManifestVersion1_1_RC4 with oras.PackManifestVersion1_1 - Add nolint comments to suppress SA1019 warnings for deprecated OPA v0.x packages - Migration to OPA v1 API requires extensive changes and should be done separately - All code compiles successfully with these changes Signed-off-by: HeerakKashyap <[email protected]>
Hey @n2h9 @leecalcote, I've updated the PR with all the fixes for the issues we were seeing. The description now explains what was broken and how I fixed it. Would appreciate a review when you have time! |
Hey hey, @HeerakKashyap hello 👋 😇 You need to take connection definition from the schema repo and not define it in meshkit. |
"github.com/open-policy-agent/opa/storage" | ||
"github.com/open-policy-agent/opa/storage/inmem" | ||
"github.com/open-policy-agent/opa/topdown/print" | ||
"github.com/open-policy-agent/opa/rego" //nolint:staticcheck // SA1019: deprecated package, migration to v1 API requires extensive changes |
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.
Do you think that this pr: #789 fixes lint issues in opa package ?
I encourage you to go and leave your thoughts on the changes in this pr 🤗 .
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 think it is unnecessary to touch this file, as you do not do anything here related to the task.
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.
Do you think that this pr: #789 fixes lint issues in opa package ?
I encourage you to go and leave your thoughts on the changes in this pr 🤗 .
Thanks for pointing this out @n2h9! Looking at PR #789, this is exactly what we needed - it's doing a proper OPA migration from v1.0.1 to v1.7.1 with clean API refactoring instead of just slapping on nolint comments. The Rego code looks much cleaner now with the new API patterns, and they're updating all the other dependencies too. This is way better than what we were trying to do in our PR. Since this PR is specifically about dependency modernization, it makes total sense to let it handle all the OPA and ORAS stuff while we keep our ConnectionDefinition PR focused on just that functionality. Much cleaner approach - thanks for letting me know... 😄
return err | ||
} | ||
defer resp.Body.Close() | ||
defer SafeClose(resp.Body) |
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 think in this file will be enough to just update switch case to handle case "connections.meshery.io" and updates to close are not related to the issue. If you think they improve the code quality, could you please create a new pr with it 😇 .
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 sure...
models/registration/utils.go
Outdated
|
||
// TODO: refactor this and use CUE | ||
func getEntity(byt []byte) (et entity.Entity, _ error) { | ||
func GetEntity(byt []byte) (et entity.Entity, _ error) { |
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 do not see any place where you use it ourtside of the package 🤷♀️ if so, let's probably update it back to be unexported?
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.
done!
models/registration/utils.go
Outdated
) | ||
|
||
// ConnectionSchemaVersion is the schema version for connection definitions | ||
const ConnectionSchemaVersion = "connections.meshery.io/v1beta1" |
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.
please also define this in the schema package 😇
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.
done!
- Revert GetEntity back to unexported getEntity - Move ConnectionSchemaVersion to corev1beta1 package - Remove changes to OPA and OCI files (unrelated to task) - Keep only ConnectionDefinition support changes Signed-off-by: HeerakKashyap <[email protected]>
545d290
to
0c6d15f
Compare
Thanks for the feedback @n2h9! I looked into using the existing For now, I've kept the This way we can move forward with the registration system while maintaining the correct architectural pattern. Would you like me to create a separate PR to the schemas repo to add the proper |
I think we need to add missing functions to connection in connection_helper.go to make it implement required interface. Use Component and Model entities (they also have xxx_helper.go file in schema repo. thank you 🙇♀️ |
Hey @n2h9! Thanks for the feedback! 👋 😅 I get ur point - we should definitely use the schemas repo definitions when possible. But I ran into a bit of a problem when I tried to add those Entity methods to the existing Here's what I found: the If you look at |
or maybe @leecalcote could help us give their opinion regarding this... 😊🙇 |
Signed-off-by: HeerakKashyap <[email protected]>" git commit --amend --signoff --no-editq1 --pretty=format:"%B"feat: Add ConnectionDefinition support to registration utilities - Fix code review issues git reset --soft HEAD~1 - Add missing ConnectionDefinition entity type constant - Create ConnectionDefinition type in core v1beta1 package - Fix hardcoded string by adding ConnectionSchemaVersion constant - Fix loop variable issue in connection registration - Remove redundant nil check in connection file handling - Ensure all code compiles successfully Signed-off-by: HeerakKashyap <[email protected]>
fb385d4
to
bf8c9a6
Compare
@n2h9 kindly let me know if the changes I did works for us 🙇 |
@HeerakKashyap, test your changes and provide confirmation of those test results. |
@leecalcote I have tested locally - it was working fine. I'll upload the test results shortly to confirm it 👍 |
Could you please also remove no-lint around opa packages import, there is a separate pr which fixes the opa packages lint errors |
Sure... |
So should I leave it for now as it is...? |
I understand we don't want a ConnectionDefinition wrapper, but I'm facing a technical constraint: |
|
Hey hey @HeerakKashyap hello 👋 🤗 ! Indeed there is a conflict 😢 , could be a blocker. We want to achieve is that the result PackagingUnit will contain the list of Thank you 🙇♀️ |
Okayy sure, I'll look into this |
Signed-off-by: HootingConjuror <[email protected]>
…conflict Signed-off-by: HeerakKashyap <[email protected]>
f27cf4c
to
c96d42e
Compare
Signed-off-by: HeerakKashyap <[email protected]>
cd9829a
to
479144d
Compare
…emas directly Signed-off-by: HeerakKashyap <[email protected]>
0c78719
to
6e9e8ab
Compare
Signed-off-by: HeerakKashyap <[email protected]>
Hey hey @HeerakKashyap hello 👋 🤗 ! You are validating data over the connectionDefinition, which is a wrapper over connection entity. |
I am working on it, I'll let u know about the same asap @n2h9. Currently busy with college exams 🥲 |
…er - remove wrapper approach and validate connection entities directly from schemas repo Signed-off-by: HeerakKashyap <[email protected]>
…ion package Signed-off-by: HeerakKashyap <[email protected]>
Hey @n2h9, Let me know if this works... |
hey hey Heerak, hello 👋 . Thank you, validation over the connection model looks good 🙂 ! Couple points:
Thank you 🤗 |
okay, i'll fix these.. |
- Remove schemas/connections/connectionDefinition.json - PackagingUnit already includes Connections field - Addresses maintainer points about not needing connection schema in meshkit Signed-off-by: HeerakKashyap <[email protected]>
…nnection-definition-final' of https://github.com/HeerakKashyap/meshkit into feat/connection-definition-final git pull origin feat/connection-definition-final# Please enter a commit message to explain why this merge is necessary,
Hey @n2h9 , is it the issue being resolved corresponding to this pr ? |
Hey hey, @HeerakKashyap 👋 hello 😇 I think that this part is still pending:
|
This PR adds support for ConnectionDefinition entities in the MeshKit registration system. Previously, when importing models that contained connection definitions, these entities were being silently ignored because the registration pipeline didn't know how to handle them.
What was broken
The main issue was in
utils/utils.go
- theFindEntityType
function was missing a case forconnections.meshery.io
, so it couldn't identify connection definition files. This meant connection definitions were being skipped during model import.What I fixed
connections.meshery.io
case toFindEntityType
inutils/utils.go
getEntity
public asGetEntity
inmodels/registration/utils.go
so it can be testedPackagingUnit
inmodels/registration/register.go
to include connections and handle their registrationmodels/registration/dir.go
models/meshmodel/registry/registry.go
ConnectionDefinition
type inmodels/meshmodel/core/v1beta1/connection.go
ConnectionSchemaVersion
constant in the corev1beta1 packageBased on reviewer feedback, I've made the following adjustments:
GetEntity
back to unexportedgetEntity
since it's not used outside the packageConnectionSchemaVersion
constant to the corev1beta1 packageTesting
I tested this with a sample ConnectionDefinition JSON file. The pipeline now works correctly:
FindEntityType
properly identifies connection definition filesgetEntity
parses them into the right typeBefore this fix, connection definitions were just being ignored. Now the registry can properly handle all entity types: components, relationships, and connections.
fixes- #762
Notes for Reviewers