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

Notebook Spawner reachable when disabled #1505

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

pnaik1
Copy link
Contributor

@pnaik1 pnaik1 commented Jul 12, 2023

Closes: #1462

Description

#1462
Not to be able to find a route to the notebookController part of the app when it is disabled.
NotebookSpawner
The empty state page
image

How Has This Been Tested?

Modifying the OdhDashboardConfig is required to test this.
Enable DS Projects spec.dashboardConfig.disableProjects = false
Disable NotebookController spec.notebookController.enable = false
You can see in DS project , you cannot find a route to notebookController

Test Impact

no test coverage

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • 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 tests & storybook 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

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Navigate to the DS Projects view, and click on the launch notebook button (both empty state and list state has the button)

You only got half of this no 3 point from the issue. Please get the empty state too.

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 12, 2023

@andrewballantyne my bad , I totally missed it

@pnaik1 pnaik1 force-pushed the issue-1462 branch 2 times, most recently from dc3cdd9 to 4c898c7 Compare July 12, 2023 15:07
@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 12, 2023

@andrewballantyne made the changes

@andrewballantyne
Copy link
Member

@andrewballantyne made the changes

@pnaik1 please update your description with another screenshot

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 12, 2023

@andrewballantyne updated with screenshot

@andrewballantyne
Copy link
Member

@pnaik1 so you want to be on the lookout for text that is confusing if we don't have that functionality. We don't have Jupyter notebook and your screenshot says to the user they can use it -- which is not true.

@kywalker-rh @kaedward thoughts on the text -- We also have two sets of text, so I bet we need to update both scenarios (with and without self provision abilities).

@kywalker-rh
Copy link

Unless its disabled by something else, I think we should have the "Launch Jupyter" link button, then the text will make sense. Is it not included for a reason?

@andrewballantyne
Copy link
Member

Unless its disabled by something else, I think we should have the "Launch Jupyter" link button, then the text will make sense. Is it not included for a reason?

@kywalker-rh this is the issue, #1462 -- If you disable the tile, we still link to it from DS Projects. This issue is addressing that gap. We shouldn't link to something if you disable the Jupyter Tile.

@kywalker-rh
Copy link

Thanks... now I know to click all the links in a PR. In this case I think we should simply remove the or launch a notebook with Jupyter text for both options of the page.

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 13, 2023

@andrewballantyne @kywalker-rh I have made the changes

Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Das tested this... the code looks good to me.

I'm going to approve. My only remark is when we get to translations, we'll have to handle this text chunking better -- but that's an unknown distant future.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, manaswinidas

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

The pull request process is described 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

@openshift-merge-robot openshift-merge-robot merged commit c1848e3 into opendatahub-io:main Jul 13, 2023
4 checks passed
@pnaik1 pnaik1 deleted the issue-1462 branch July 22, 2024 14:23
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.

[Bug]: Notebook Spawner reachable when disabled
5 participants