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.

Sign up for GitHub

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

Jump to bottom

[chore] Improve MeshModels models retrieval #375

Merged
merged 17 commits into from
Sep 28, 2023
Merged

[chore] Improve MeshModels models retrieval #375

merged 17 commits into from
Sep 28, 2023
+121 −25

Conversation

theBeginner86
Copy link
Member

@theBeginner86 theBeginner86 commented Sep 27, 2023 •
edited
Loading

Description

This PR fixes # meshery/meshery#8929

Fixes:

  1. Fix unwanted re-registering of hosts. Earlier same host was being re-registered for each component. So we had about ~4000 entries for ArtifactHub. Now it would be just one.
  2. Ensure Models are too registered as an entity in registries table. Also, made model compatible with Entity interface{}.
  3. Enhanced GetModels handler to do the heavy lifting and return all the necessary info to client instead.

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Sorry, something went wrong.

@theBeginner86
update host registration:
ddd2f71 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86 theBeginner86 added kind/chore Necessary task pr/draft WIP/Draft pull request labels Sep 27, 2023
@theBeginner86
fix condition when host is found
41f6e46 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
rm logs
10257d0 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
rm debug log
ee84552 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86 theBeginner86 mentioned this pull request Sep 27, 2023
Merged
1 task
@theBeginner86
return model id for registration component
c98de33 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
define modeldef as entity type
3053ef0 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
add registry models as entity handler
c9fc626 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
return modeid while registering policy
4bab259 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
return modeid while registering relationship
a51b1bf 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
register model into registry for relationship/policy registeration pr…
ff63b38 
…ocess

Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
fix model type
4577fd6 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
make model entity interface compatible
a85f6ae 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
return host metdata with models
9160e59 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86 theBeginner86 removed the pr/draft WIP/Draft pull request label Sep 28, 2023
@theBeginner86
rm comment
6435f44 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
fix adapter issue
6aa7241 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86
Copy link
Member Author

theBeginner86 commented Sep 28, 2023 •
edited
Loading

I'm unclear of what is the meaning of hostname in hosts table. Does it mean name of host or the hostname (the one used in networking)?
I'm asking this because for registrants like ArtifactHub or Kubernetes the value of hostname is artifact hub and kubernetes respectively but for Meshery Adapters its localhost (considering I was running adapter locally).

Isn't this inconsistent?

@theBeginner86
lint
5fc7b5c 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
theBeginner86
theBeginner86 commented Sep 28, 2023
View reviewed changes
models/meshmodel/registry/host.go
@@ -34,8 +59,8 @@ func (h *Host) AfterFind(tx *gorm.DB) error {
h.IHost = ArtifactHub{}
case "kubernetes":
h.IHost = Kubernetes{}
default:
return ErrUnknownHost(errors.New("unable to find compatible host for the component"))
default: // do nothing if the host is not pre-unknown. Currently adapters fall into this case.
Copy link
Member Author

Choose a reason for hiding this comment

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

This change relates to the question I asked here: #375 (comment)

@leecalcote
Copy link
Member

I'm unclear of what is the meaning of hostname in hosts table. Does it mean name of host or the hostname (the one used in networking)? I'm asking this because for registrants like ArtifactHub or Kubernetes the value of hostname is artifact hub and kubernetes respectively but for Meshery Adapters its localhost (considering I was running adapter locally).

Isn't this inconsistent?

Answered verbally.

leecalcote
leecalcote approved these changes Sep 28, 2023
View reviewed changes
models/meshmodel/registry/registry.go
@@ -47,6 +49,36 @@ type RegistryManager struct {
db *database.Handler //This database handler will be used to perform queries inside the database
}

func registerModel(db *database.Handler, regID, modelID uuid.UUID) error {
Copy link
Member

Choose a reason for hiding this comment

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

Need function description.

@theBeginner86
Update models/meshmodel/registry/registry.go
f6ed5a4 
Signed-off-by: Pranav Singh <pranavsingh02@hotmail.com>
@theBeginner86 theBeginner86 merged commit 46e722c into meshery:master Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leecalcote leecalcote

@MUzairS15 MUzairS15

Assignees
No one assigned
Labels
kind/chore Necessary task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@theBeginner86 @leecalcote