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 User Data Collection checkbox #404

Merged
merged 4 commits into from Aug 22, 2022

Conversation

mlassak
Copy link
Contributor

@mlassak mlassak commented Aug 8, 2022

Resolves: #354

Description

Replaced the default null state of User Data Collection checkbox. Additionally, fixed a potential cause of exceptions due to missing null checks in clusterSettingsUtils.ts when dashboard config notebook controller spec is not present.

How Has This Been Tested?

Tested locally

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

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.

This didn't entirely solve the bug. If you lack some of the resources used in the updateClusterSettings call, it breaks the API call (doesn't end it, just fails to ever complete). This can lead to issues with the UI since the requests browsers make are limited concurrently and thus you can block all other API calls if the user spams the button.

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.

I suspect your cluster has resources that mine does not.

We should be able to gracefully handle the lack of resources. Otherwise we need to present this info upfront to the user that they do not have a proper configuration. I think in all cases in this clusterSettingsUtils, we can gracefully handle 404s.

My error is (e.body.message): message: 'configmaps "notebook-controller-culler-config" not found', 404 status code.

Which looks to be the scenario my culling is disabled, delete configmap -- definitely don't need to crash and burn on that call. (line 51)

@samuelvl
Copy link
Contributor

/retest

@maroroman
Copy link
Contributor

Sorry @andrewballantyne did not mean that re-review yet

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.

hmmm... we don't save it (if you lack the files) and say we saved it 😞 My catch-all logic sorta made the call do nothing, but look like it succeeded. Gosh, I really don't want to go overboard with refactoring this function 🤔

Okay... so this is a pre-existing problem. We fixed my concern about it not updating properly and crashing when things went off the rails. Let's go with this, and we can work to improve this method. Ideally we take this to the client shortly after #279

@openshift-ci openshift-ci bot added the lgtm label Aug 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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 6eed8c2 into opendatahub-io:main Aug 22, 2022
LaVLaS pushed a commit to LaVLaS/odh-dashboard that referenced this pull request Aug 26, 2022
* fix default udc checkbox state

* updated updateClusterSettings()
LaVLaS pushed a commit to red-hat-data-services/odh-dashboard that referenced this pull request Aug 31, 2022
* fix default udc checkbox state

* updated updateClusterSettings()
strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023
* fix default udc checkbox state

* updated updateClusterSettings()
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.

[KFNBC/JH]: Usage Data Collection checkbox is in a weird state by default and cannot be "enabled"
5 participants