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

Give default title and action label to the application enable modal #3648

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Jan 15, 2025

JIRA: https://issues.redhat.com/browse/RHOAIENG-425

Description

In OdhApplication CRD, the field spec.enable may not contain title and actionLabel fields. Give them default value to show the enable modal correctly. Default fallback title is spec.displayName and default fallback actionLabel is Enable

Screenshot 2025-01-15 at 1 37 06 PM

How Has This Been Tested?

Import the following YAML to your cluster using OpenShift console opendatahub namespace (or the namespace where you deployed the ODH dashboard)

apiVersion: dashboard.opendatahub.io/v1
kind: OdhApplication
metadata:
  annotations:
    opendatahub.io/categories: 'Data management,Data preprocessing,Model training'
  name: mlflow-server
  labels:
    app: odh-dashboard
spec:
  enable:
    validationConfigMap: mlflow-enable
  img: '<svg width="24" height="25" viewBox="0 0 24 25" fill="none" xmlns="http://www.w3.org/2000/svg"> <path d="M18.9509 2.17634C14.0635 -1.24345 7.40926 -0.571051 3.30485 3.75735C-0.799556 8.08576 -1.11776 14.7663 2.55667 19.4652L6.22524 16.7724C4.40423 14.5134 4.03437 11.4124 5.27298 8.78843C6.51159 6.16448 9.1408 4.47914 12.0422 4.44929L11.9554 7.31581L18.9509 2.17634Z" fill="#43C9ED"/> <path d="M21.6639 4.85176C21.5423 4.68382 21.4149 4.51878 21.2846 4.35953L17.7521 6.96546C19.7463 9.17232 20.2637 12.3424 19.0746 15.0688C17.8856 17.7952 15.2104 19.5728 12.2362 19.6129L12.3231 16.7493L5.24945 21.9611C10.0971 25.2255 16.5896 24.5337 20.6406 20.3213C24.6917 16.1089 25.1295 9.59437 21.6784 4.87782L21.6639 4.85176Z" fill="#0194E2"/> </svg>'
  getStartedLink: 'https://mlflow.org/docs/latest/quickstart.html'
  route: test
  displayName: MLflow
  kfdefApplications: []
  support: third party support
  csvName: ''
  provider: MLflow
  routeNamespace: test
  docsLink: 'https://mlflow.org/docs/latest/index.html'
  quickStart: ''
  getStartedMarkDown: |-
    # MLFlow
    MLflow is an open source platform for managing the end-to-end machine learning lifecycle. It tackles four primary functions:
    - Tracking experiments to record and compare parameters and results (MLflow Tracking). - Packaging ML code in a reusable, reproducible form in order to share with other data scientists or transfer to production (MLflow Projects). - Managing and deploying models from a variety of ML libraries to a variety of model serving and inference platforms (MLflow Models). - Providing a central model store to collaboratively manage the full lifecycle of an MLflow Model, including model versioning, stage transitions, and annotations (MLflow Model Registry).
  description: MLflow is an open source platform for managing the end-to-end machine learning lifecycle.
  category: Self-managed

Validate if the title is Enable {displayName} and the button is showing Enable as expected.

Test Impact

N/A, we don't have a test for the enable modal because it's an old component, not sure if it worth the effort to write tests for that.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from lucferbux and pnaik1 January 15, 2025 18:40
Copy link
Contributor

openshift-ci bot commented Jan 15, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.02%. Comparing base (bfe6c12) to head (998f891).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ntend/src/pages/exploreApplication/EnableModal.tsx 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3648      +/-   ##
==========================================
- Coverage   84.05%   84.02%   -0.03%     
==========================================
  Files        1432     1432              
  Lines       33479    33481       +2     
  Branches     9279     9281       +2     
==========================================
- Hits        28140    28134       -6     
- Misses       5339     5347       +8     
Files with missing lines Coverage Δ
...ntend/src/pages/exploreApplication/EnableModal.tsx 32.75% <0.00%> (-1.17%) ⬇️

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfe6c12...998f891. Read the comment docs.

@rsun19
Copy link
Contributor

rsun19 commented Jan 16, 2025

/lgtm

The enable button appears on my machine, and code looks good

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

Successfully merging this pull request may close these issues.

2 participants