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 return type when creating dashboard config #3598

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

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Dec 19, 2024

https://issues.redhat.com/browse/RHOAIENG-15388

Description

Fixes error Cannot read properties of undefined (reading 'notebookController')

The resourceUtils.ts#createDashboardCR function returned an array instead of the dashboard config itself, therefore when attempting to read spec.notebookController from the config, it would fail since spec is undefined on the array. At the same time, also returned the blankDashboardCR in case creation were to fail.

This points to a bigger issue where our tsconfig needs to be made strict.

How Has This Been Tested?

  • Spin down the dashboard deployment or stop your local backend server.
  • Delete the OdhDashboardConfig
  • Start the dashboard backend
  • Immediately hit the frontend URL.
  • Page should load without error.

Test Impact

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 added the do-not-merge/work-in-progress This PR is in WIP state label Dec 19, 2024
Copy link
Contributor

openshift-ci bot commented Dec 19, 2024

[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 ask for approval from christianvogt. 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

@christianvogt christianvogt force-pushed the health-checks branch 3 times, most recently from 81aa709 to 197c3fd Compare December 19, 2024 21:26
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.15%. Comparing base (4260e3e) to head (197c3fd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3598      +/-   ##
==========================================
- Coverage   85.17%   85.15%   -0.03%     
==========================================
  Files        1393     1393              
  Lines       31942    31942              
  Branches     8958     8958              
==========================================
- Hits        27208    27201       -7     
- Misses       4734     4741       +7     

see 3 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 4260e3e...197c3fd. Read the comment docs.

@christianvogt christianvogt force-pushed the health-checks branch 3 times, most recently from 9b73d1b to de681cd Compare December 20, 2024 16:10
@christianvogt christianvogt changed the title [WIP] health check to include dashboard config fix return type when creating dashboard config Dec 20, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Dec 20, 2024
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.

1 participant