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

update fields to retrieve HostID and name #332

Closed
wants to merge 18 commits into from

Conversation

Freedisch
Copy link
Member

@Freedisch Freedisch commented Jul 22, 2023

Description

This PR fixes Update fields for Models, Relationships, and components to display host Id and name

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@welcome
Copy link

welcome bot commented Jul 22, 2023

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.

@leecalcote
Copy link
Member

@zakisk

Copy link
Contributor

@zakisk zakisk left a comment

Choose a reason for hiding this comment

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

@Freedisch code in terms of solving issue is good but linting must be added

models/meshmodel/core/v1alpha1/models.go Outdated Show resolved Hide resolved
Signed-off-by: Freedisch <[email protected]>
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.

@Freedisch Have you verified your changes? Do you get HostID, HostName, DisplayHostName when you query for model?
You also need to modify GetModel IMO.

@Freedisch
Copy link
Member Author

Freedisch commented Jul 25, 2023

@MUzairS15 , I already modified GetModel it's in another in this PR #8257,
here the body response
image

Regarding the model, are the available models registered under a host/or registrant?
image

@MUzairS15
Copy link

MUzairS15 commented Jul 25, 2023

@MUzairS15 , I already modified GetModel it's in another in this PR #8257, here the body response image

Regarding the model, are the available models registered under a host/or registrant? image

This is what I was referring to, the second screenshot, hostname and id are empty, you need to modify getmodels

@Freedisch
Copy link
Member Author

@MUzairS15
image

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.

@Freedisch I understand that you are retrieving registrants and assigning host details (in your meshery PR), but host details are now included in Model constructs so why not set host details while retrieving models? https://github.com/meshery/meshkit/pull/332/files#diff-69b1a3b3507297713ffe3c635a25bbf8a8807b7a2c316a8bed048c222fde7b55R194

@Freedisch
Copy link
Member Author

@Freedisch I understand that you are retrieving registrants and assigning host details (in your meshery PR), but host details are now included in Model constructs so why not set host details while retrieving models? https://github.com/meshery/meshkit/pull/332/files#diff-69b1a3b3507297713ffe3c635a25bbf8a8807b7a2c316a8bed048c222fde7b55R194

I actually noticed that and updated it, I will fix the conflicts and push it for reviews

@leecalcote
Copy link
Member

merge conflict

Signed-off-by: Freedisch <[email protected]>
Signed-off-by: Freedisch <[email protected]>
@Freedisch
Copy link
Member Author

Freedisch commented Aug 3, 2023

@MUzairS15, thoughts? i updated the pr

PS: Refer to this one #337

@leecalcote
Copy link
Member

Rady for re-review, @MUzairS15

@Freedisch
Copy link
Member Author

@leecalcote, sorry forgot to close this one. The changes here have been already merged here #337

@Freedisch Freedisch closed this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants