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

Fix ignored models not showing up in Registry and add option to toggle visibility of ignored models #673

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

Conversation

riyaa14
Copy link
Contributor

@riyaa14 riyaa14 commented Feb 10, 2025

Description

This PR fixes Meshery issue #13317

Notes for Reviewers

  • currently, status is set to null string, and thus both enabled and ignored models are being fetched
  • if status is set to enabled (TODO: a toggle feature on UI, see issue #13662), then ignored models would be hidden

Before -
Ignored models were not showing up on registry page

After -

Screen.Recording.2025-02-19.at.1.43.05.PM.mov

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Riya Garg <riyag1452003@gmail.com>
Signed-off-by: Riya Garg <riyag1452003@gmail.com>
@riyaa14 riyaa14 requested a review from aabidsofi19 February 10, 2025 21:01
@riyaa14 riyaa14 removed the pr/do-not-merge PRs not ready to be merged label Feb 10, 2025
Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Clarification: Ignored models won’t be hidden in the Registry page.

We want to be able to see both Ignored and Enabled models. Users should be able to mark a model as either Ignored or Enabled. Clients (other pages or other programs) can choose to display or not display / use or not use a model based on whether it is enabled or ignored. But, the Registry page should always show all models. Make sense? (edited)

Please verify whether Meshery Server processes (or does not process) Designs that have components of models set to Ignored.

Remind me: I recommended that you partner with a frontend engineer to see the changes through for complete functionality, is that right? If this functionality (this PR) is ready for review, was there no frontend change needed? Or is the full functionality being separately tracked? I don’t see another linked issue #…

@leecalcote
Copy link
Member

Is there a screenshot or video recording of the revised behavior?

@leecalcote
Copy link
Member

leecalcote commented Feb 10, 2025

Did you clarify the difference between PublishToRegistry and Status? No changes are needed in schema, correct?

@riyaa14
Copy link
Contributor Author

riyaa14 commented Feb 19, 2025

@leecalcote ,

I have attached a screen recording showing how after this change, we'll be able to see both ignored and enabled models on Registry Page

For the UI toggle feature, I have opened an issue which is linked in description of this PR as well -> issue#13662. This PR is not dependent on UI toggle feature for show/hide ignored models, and hence this PR can be merged so that user can see both enabled and ignored models and toggle between them.

Please verify whether Meshery Server processes (or does not process) Designs that have components of models set to Ignored.

Yes designs that have components of models set to ignored are shown, they aren't hidden.

@riyaa14
Copy link
Contributor Author

riyaa14 commented Feb 19, 2025

Did you clarify the difference between PublishToRegistry and Status? No changes are needed in schema, correct?

I haven't made any schema changes in this PR

but I see that PublishToRegistry and status are related. When creating model definition, if publishToRegitry is true, then status is set to enabled, else it is set to ignored
Screenshot 2025-02-19 at 2 31 53 PM

@souvikinator
Copy link
Contributor

souvikinator commented Feb 19, 2025

Makes sense. By default, the get should return all records and only filter by status if a specific value is provided. This looks good.
@riyaa14 PublishToRegistry is in the integration spreadsheet that is set to true on certain criteria i.e model has components, is categorized, etc. Here's the formula:
image

Even uncategorized models will not be registered.

if mf.Status != "" {
status = mf.Status
finder = finder.Where("model_dbs.status = ?", mf.Status)
Copy link
Member

Choose a reason for hiding this comment

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

If there is no status filter set, then status can be excluded from the where clause entirely, yeah?

Copy link
Contributor Author

@riyaa14 riyaa14 Feb 19, 2025

Choose a reason for hiding this comment

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

yes, if status is an empty string (i.e not set), we skip filtering using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI not showing updated state of model in Registry
3 participants