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

Improve update call in editing workbench #3168

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

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Sep 6, 2024

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

Description

After discussions within the eng team, we decided not to go with the UX design and simply create some actions in the alert to allow the user to override the value and force update the workbench or abandon the value and refresh the page.

The PR does the following things:

  1. Added a merge patch method to the k8s API so we can merge patch a resource
  2. Create a new method called mergePatchUpdateNotebook to override the values in the workbench using merge-patch
  3. Dry-run the current method updateNotebook (which will use PUT method instead of PATCH), if it generates a 409 conflict because the workbench is updated on the cluster, then prompt 2 actions to allow users to force update the workbench or refresh the page
Screenshot 2024-09-06 at 4 29 02 PM

How Has This Been Tested?

  1. Create a workbench
  2. Open the workbench editing page (Tab 1)
  3. Open a new browser tab (Tab 2)
  4. Edit the workbench in Tab 2 and save
  5. Get back to Tab 1 and click "Update" and you can see the error
  6. Test the Force update and Refresh buttons, the first one will push all your changes in Tab 1 through and override the changes you made in Tab 2, and Refresh will refresh the whole page and fetch the latest notebook values

Test Impact

Update some test cases

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

Copy link
Contributor

openshift-ci bot commented Sep 6, 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 assign gkrumbach07 for approval. For more information see the Kubernetes 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

@DaoDaoNoCode DaoDaoNoCode force-pushed the jira-rhoaieng-1157 branch 2 times, most recently from ddbd905 to 89bb7dd Compare September 6, 2024 22:16
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 78.26087% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.47%. Comparing base (da0aa67) to head (3af201e).

Files with missing lines Patch % Lines
...c/pages/projects/screens/spawner/SpawnerFooter.tsx 73.52% 9 Missing ⚠️
frontend/src/api/k8sUtils.ts 88.88% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3168      +/-   ##
==========================================
- Coverage   85.47%   85.47%   -0.01%     
==========================================
  Files        1279     1279              
  Lines       28161    28184      +23     
  Branches     7525     7530       +5     
==========================================
+ Hits        24070    24089      +19     
- Misses       4091     4095       +4     
Files with missing lines Coverage Δ
frontend/src/api/k8s/notebooks.ts 97.52% <100.00%> (+0.06%) ⬆️
frontend/src/api/k8sUtils.ts 95.45% <88.88%> (-4.55%) ⬇️
...c/pages/projects/screens/spawner/SpawnerFooter.tsx 73.84% <73.52%> (-3.47%) ⬇️

... and 2 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 da0aa67...3af201e. Read the comment docs.

@DaoDaoNoCode DaoDaoNoCode requested review from Gkrumbach07 and christianvogt and removed request for dpanshug and ppadti September 12, 2024 14:17
@DaoDaoNoCode
Copy link
Member Author

/hold
Wait until #3167 gets merged, I guess there will be some conflicts and it's easier from my side to fix them.

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Sep 13, 2024
@DaoDaoNoCode
Copy link
Member Author

/unhold
Conflict solved.

@openshift-ci openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Sep 16, 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