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

Enable pipeline experiments in dashboard config #2892

Conversation

Gkrumbach07
Copy link
Member

closes: https://issues.redhat.com/browse/RHOAIENG-3826

Description

  • set experiments feature flag on instead
  • moved all navigation to go to /pipelines and not /pipelineRuns
  • created the runs based on version details page

How Has This Been Tested?

  • test all routes and make sure they dont go to /pipelineRuns
  • test that version details page shows only runs and jobs from its version and the table does not have a version col

Test Impact

will need to updated routes tests

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).

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 requested review from alexcreasy and mturley June 7, 2024 22:11
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 7, 2024
@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch from c65a910 to b6502bd Compare June 7, 2024 22:19
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 7, 2024
@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch 5 times, most recently from edbab67 to 0793a44 Compare June 11, 2024 14:08
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

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

Project coverage is 78.33%. Comparing base (ba94286) to head (7d528ef).
Report is 16 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2892      +/-   ##
==========================================
- Coverage   78.46%   78.33%   -0.13%     
==========================================
  Files        1127     1128       +1     
  Lines       23943    24034      +91     
  Branches     6033     6076      +43     
==========================================
+ Hits        18786    18827      +41     
- Misses       5157     5207      +50     
Files Coverage Δ
frontend/src/api/pipelines/custom.ts 97.70% <100.00%> (+0.23%) ⬆️
...rc/concepts/pipelines/apiHooks/usePipelineQuery.ts 100.00% <ø> (ø)
.../concepts/pipelines/apiHooks/usePipelineRunJobs.ts 91.66% <100.00%> (+1.66%) ⬆️
...src/concepts/pipelines/apiHooks/usePipelineRuns.ts 90.62% <100.00%> (+1.33%) ⬆️
.../pipelinesDetails/pipeline/PipelineTaskDetails.tsx 75.00% <ø> (-1.20%) ⬇️
...sDetails/pipelineRun/PipelineRunDetailsActions.tsx 56.25% <100.00%> (+0.93%) ⬆️
...es/content/tables/pipelineRun/PipelineRunTable.tsx 80.15% <100.00%> (+1.29%) ⬆️
...content/tables/pipelineRun/PipelineRunTableRow.tsx 93.47% <100.00%> (+0.29%) ⬆️
...ent/tables/pipelineRun/PipelineRunTableToolbar.tsx 78.94% <100.00%> (-4.84%) ⬇️
...tent/tables/pipelineRunJob/PipelineRunJobTable.tsx 97.72% <100.00%> (+0.35%) ⬆️
... and 17 more

... and 18 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 ba94286...7d528ef. Read the comment docs.

@DaoDaoNoCode
Copy link
Member

I can still access the global pipeline runs page via URL, should we also remove that and make it a 404 since we don't use that page anymore?

Screenshot 2024-06-11 at 12 48 54 PM

@Gkrumbach07
Copy link
Member Author

Andrew said to keep it around. as long as we cant get to it from supported flows @DaoDaoNoCode

Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

When you create a run from the pipeline version view runs page, the pipeline version is not prefilled in the create run page form.

Screenshot 2024-06-11 at 1 11 57 PM Screenshot 2024-06-11 at 1 12 44 PM

@Gkrumbach07
Copy link
Member Author

@DaoDaoNoCode I updated the PR

@DaoDaoNoCode
Copy link
Member

@Gkrumbach07 Can you rebase the PR?

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 12, 2024
@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch from 0793a44 to 486ad5a Compare June 12, 2024 15:17
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 12, 2024
@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch from 486ad5a to 81480e2 Compare June 12, 2024 16:23
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

The link here on execution details page needs to be updated, otherwise it will navigate to old global pipeline run details page.

@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch from 81480e2 to 9e3a6fd Compare June 12, 2024 16:59
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

Tested all the routes I could think of, it works well.
/lgtm

chore: Update URLs for cloning pipeline runs and viewing pipeline runs and schedules

create run and job auto add now

Refactor PipelineTaskDetails component to remove unused code and improve readability

chore: Update route for pipeline run details namespace on execution details page

chore: Update route for pipeline run details namespace on execution details page

chore: Update route for pipeline run details namespace on execution details page
@Gkrumbach07 Gkrumbach07 force-pushed the task/RHOAIENG-3826-remove-old-runs-pages-and-routing-and-experiments branch from 9e3a6fd to 7d528ef Compare June 12, 2024 22:24
@openshift-ci openshift-ci bot removed the lgtm label Jun 12, 2024
@Gkrumbach07
Copy link
Member Author

@DaoDaoNoCode I ended up finding a clean solution for it

Copy link
Contributor

@dpanshug dpanshug left a comment

Choose a reason for hiding this comment

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

/lgtm

@Gkrumbach07
Copy link
Member Author

merging this

/approve

Copy link
Contributor

openshift-ci bot commented Jun 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaoDaoNoCode, dpanshug, 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

@openshift-merge-bot openshift-merge-bot bot merged commit 595ace5 into opendatahub-io:main Jun 13, 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.

4 participants