Skip to content
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

Move MeshModel to Model. #468

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gyohuangxin
Copy link
Member

Description

This PR fixes #467

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Copy link

welcome bot commented Feb 28, 2024

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@@ -42,7 +42,7 @@ type ComponentDefinition struct {
UpdatedAt time.Time `json:"-"`
}
type ComponentDefinitionDB struct {
ID uuid.UUID `json:"id"`
ID uuid.UUID `json:"componentDefinitionId"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MUzairS15 @leecalcote To fix the golangci-lint issue, I renamed the Json tag. Will it have an impact on its dependencies?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yashsharma1911 Could confirm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yashsharma1911 may be slow to response, therefore I revert this unpredictable modification so that we can keep going. // @MUzairS15 @leecalcote

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RipulHandoo, I wonder if you might be familiar here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leecalcote @MUzairS15 Can we go ahead, and I can create another issue mentioning @Yashsharma1911 @RipulHandoo to track it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue created: #469

@@ -60,7 +60,7 @@ type Model struct {
}

type ModelDB struct {
ID uuid.UUID `json:"id"`
ID uuid.UUID `json:"modelDBId"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This will be a breaking change.

models/controllers/meshsync.go Outdated Show resolved Hide resolved
models/controllers/meshsync.go Outdated Show resolved Hide resolved
models/controllers/meshery_broker.go Outdated Show resolved Hide resolved
models/controllers/meshery_broker.go Outdated Show resolved Hide resolved
@gyohuangxin
Copy link
Member Author

@leecalcote Do you have any comments?

@@ -35,7 +35,7 @@ type ComponentParameter struct {
Description *string `json:"description,omitempty"`
}

const MesheryAnnotationPrefix = "design.meshmodel.io"
const MesheryAnnotationPrefix = "design.model.io"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MUzairS15 this is to be design.meshery.io, right?

@leecalcote
Copy link
Member

@leecalcote Do you have any comments?

Thank you for asking. I only have two:

  1. Is regarding a potential existing bug in the namspacing of the MesheryAnnotationPrefix, which might need to be addressed in Meshery Server as well.
  2. Which leads us to my second comment, which is: @MUzairS15 what do we need to do in order to ensure that Server and the mesheryctl registry command and workflows, and the meshery/schemas are all in sync as we move forward with what will be a series of changes? Is there a specific sequence of next steps to follow? @gyohuangxin, I'm trying to remember where it is that I proposed what this sequence might be. Do you recall, by chance?

@gyohuangxin
Copy link
Member Author

Which leads us to my second comment, which is: @MUzairS15 what do we need to do in order to ensure that Server and the mesheryctl registry command and workflows, and the meshery/schemas are all in sync as we move forward with what will be a series of changes? Is there a specific sequence of next steps to follow? @gyohuangxin, I'm trying to remember where it is that I proposed what this sequence might be. Do you recall, by chance?

IMO, the dependencies sequence should be meshkit <- meshery-adapter-library <- Meshery components (Adapters, Mesheryctl... )

In order to make the transformation seamlessly, we can create a release of meshkit, then validate the meshery-adapter-library can work if update the meshkit version. Then we can create the release of meshery-adapter-library to validate other Meshery components base on that. This is a process that requires care and patience. @MUzairS15 Please correct me if I missed something.

@leecalcote
Copy link
Member

leecalcote commented Mar 7, 2024

Here's a related / similar chore - meshery/meshery#9969

@leecalcote
Copy link
Member

Which leads us to my second comment, which is: @MUzairS15 what do we need to do in order to ensure that Server and the mesheryctl registry command and workflows, and the meshery/schemas are all in sync as we move forward with what will be a series of changes? Is there a specific sequence of next steps to follow? @gyohuangxin, I'm trying to remember where it is that I proposed what this sequence might be. Do you recall, by chance?

IMO, the dependencies sequence should be meshkit <- meshery-adapter-library <- Meshery components (Adapters, Mesheryctl... )

In order to make the transformation seamlessly, we can create a release of meshkit, then validate the meshery-adapter-library can work if update the meshkit version. Then we can create the release of meshery-adapter-library to validate other Meshery components base on that. This is a process that requires care and patience. @MUzairS15 Please correct me if I missed something.

This is making sense to me, @gyohuangxin. @MUzairS15, sound about right?

@MUzairS15
Copy link

@gyohuangxin Yes, its correct.
AdapterLibrary - adapters
Meshery Server, mesheryctl.

While adapter-library and server can be done in parallel.

@MUzairS15
Copy link

While design.meshery.io change will affect UI clients.

I propose the change for this to be done, when the first iteration of the refactoring from meshmodels to models is complete.

I'll create a issue to track this item.

@MUzairS15
Copy link

@gyohuangxin Since meshkit undergoes a release often, and merging the PR would block subsequent releases and upgrades.

Will you first start the migration in Meshery Server by using this branch as local go.mod reference?
When the server PR is up we can merge both of them.

@gyohuangxin
Copy link
Member Author

@gyohuangxin Since meshkit undergoes a release often, and merging the PR would block subsequent releases and upgrades.

Will you first start the migration in Meshery Server by using this branch as local go.mod reference?
When the server PR is up we can merge both of them.

@MUzairS15 Got it. Will work on that.

@leecalcote
Copy link
Member

Uh-oh. A merge conflict popped up.

Copy link

stale bot commented Apr 28, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Apr 28, 2024
@Yashsharma1911 Yashsharma1911 added issue/willfix This issue will be worked on and removed issue/stale Issue has not had any activity for an extended period of time labels Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/willfix This issue will be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from MeshModel to Model
4 participants