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

[RHOAIENG-4999] Details page for archived runs #2833

Merged
merged 1 commit into from
May 30, 2024

Conversation

jenny-s51
Copy link
Contributor

Closes https://issues.redhat.com/browse/RHOAIENG-4999

Description

  • Adds a label indicating the run is archived in the run title (see mocks)
  • Adds dropdown actions for an archived run

How Has This Been Tested?

  1. Experiments and Runs
  2. Select a run
  3. Toggle dropdown in upper right
  4. Test that dropdown actions are in parity with the mocks and the methods work as expected.

Test Impact

Updated PipelinesTopology.cy.ts.

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 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 (find relevant UX in the SMEs section).
Screenshot 2024-05-21 at 3 27 41 PM

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

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 77.43%. Comparing base (c9caed3) to head (ede387d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2833      +/-   ##
==========================================
- Coverage   77.48%   77.43%   -0.05%     
==========================================
  Files        1110     1110              
  Lines       23482    23515      +33     
  Branches     5917     5932      +15     
==========================================
+ Hits        18196    18210      +14     
- Misses       5286     5305      +19     
Files Coverage Δ
...s/content/tables/experiment/ExperimentTableRow.tsx 100.00% <100.00%> (ø)
.../concepts/pipelines/context/usePipelineAPIState.ts 83.33% <ø> (ø)
frontend/src/api/pipelines/custom.ts 97.46% <50.00%> (-1.24%) ⬇️
.../content/pipelinesDetails/PipelineDetailsTitle.tsx 92.85% <66.66%> (-7.15%) ⬇️
...ipelinesDetails/pipelineRun/PipelineRunDetails.tsx 81.53% <40.00%> (-2.07%) ⬇️
...sDetails/pipelineRun/PipelineRunDetailsActions.tsx 55.31% <46.66%> (-12.69%) ⬇️

... and 1 file 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 c9caed3...ede387d. Read the comment docs.

@jenny-s51 jenny-s51 force-pushed the iss4999 branch 2 times, most recently from 5d3702b to 65f06dc Compare May 22, 2024 15:53
@jpuzz0
Copy link
Contributor

jpuzz0 commented May 23, 2024

"Clone" vs "Duplicate" was discussed here a few months ago. Unless something has changed, I believe the intention was to keep the "Duplicate" wording.

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thank you for providing clarity on this @jpuzz0 - based on that discussion it looks like the mocks are out of date, will revert back to Duplicate. cc @yannnz @xianli123

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Following the conversation in the demo of this work during the Pipelines Review meeting this morning, based on feedback from @yannnz we want to disable the Restore button on Archived runs, and add a tooltip as shown below.
Screenshot 2024-05-23 at 11 13 57 AM

It was also noted by Fede that, when navigating to an archived experiment from "Experiments and Runs", we want to set the Archived tab as the active tab:
Screenshot 2024-05-23 at 11 17 31 AM

@jenny-s51 jenny-s51 force-pushed the iss4999 branch 2 times, most recently from 66d3c85 to a4d4e74 Compare May 23, 2024 20:37
@Gkrumbach07
Copy link
Member

Need to verify this

An active run cannot be deleted. Should remove the Delete action in the Action dropdown.
Miss Retry action.

@jenny-s51 jenny-s51 force-pushed the iss4999 branch 2 times, most recently from ead6fef to 72f359d Compare May 29, 2024 19:58
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label May 29, 2024
Copy link
Contributor

openshift-ci bot commented May 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07

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

add archived labels and additional dropdown items

format, update tests

update test

update instances of Duplicate to Clone

fix lint

PR feedback: reverts Clone, disables restore on archived run, fixes route from archived experiment

PR feedback: fix route, remove helper functions

fix tests

update ExperimentTableRow logic to render dropdown items correctly, use StorageStateKF instead of location'

add dropdown option to archive

fix error

fix linting error

apply Gage diff
@Gkrumbach07
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 4aa0a8c into opendatahub-io:main May 30, 2024
8 checks passed
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.

3 participants