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

add error status popover in the pipleline, pipelinerun and taskrun list #7160

Merged

Conversation

karthikjeeyar
Copy link
Contributor

@karthikjeeyar karthikjeeyar commented Nov 9, 2020

Fixes:
https://issues.redhat.com/browse/ODC-5061
https://issues.redhat.com/browse/ODC-5099

Analysis:

  1. Error status field in the Pipeline, PLR and TR list component does not have popover to view the error and link
  2. Log snippet error in PLR details page ( Incorrect namespace was passed causing the snippet to show pod not found error
    always)
  3. Incorrect task name in taskrun list component

Solution Description:

  1. Add status popover (followed the convention mentioned here)
  2. Correct namespace is passed to the RunDetailsErrorLog component
  3. Use correct task name from the pipeline

Screen shots / Gifs for design review:

  1. Failed status field to have popover:
    image

  2. Log snippet in PLR details page
    image (2)

  3. Correct Task name in the taskRun list Component
    image (3)

status-popover

Unit test coverage report:

image

Test setup:

  1. Use Git add flow to create a pipeline and start it.
  2. Click on the pipeline tab on the navigation menu

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@andrewballantyne @bgliwa01 @openshift/team-devconsole-ux

@bgliwa01
Copy link

bgliwa01 commented Nov 9, 2020

The arrow of the popover should align with whatever the user clicks, so in this case it should be lined up with failed

@nemesis09
Copy link
Contributor

/retest

@karthikjeeyar
Copy link
Contributor Author

@bgliwa01 Aligned the popover with the status. Updated the code and screenshots.

@@ -46,7 +46,7 @@ export const PipelineRunDetails: React.FC<PipelineRunDetailsProps> = ({ obj: pip
</dl>
<RunDetailsErrorLog
logDetails={getPLRLogSnippet(pipelineRun)}
namespace={pipelineRun.metadata.name}
namespace={pipelineRun.metadata.namespace}
Copy link
Member

Choose a reason for hiding this comment

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

😄 👍

import { PipelineRunModel } from '../../../models';
import PipelineResourceStatus from './PipelineResourceStatus';
import StatusPopoverContent from './StatusPopoverContent';
import { DASH } from '@console/shared';
Copy link
Member

Choose a reason for hiding this comment

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

nit: Move absolute import above relative

Comment on lines 1 to 8
.odc-statuspopover-content {
min-height: 175px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Understand that the content could not have a full dynamic high because of the left-arrow, but here is an recommendation that we could grow the code snippet to the available space. What do you think?

Before:
image

With the following flex layout:
image

Suggested change
.odc-statuspopover-content {
min-height: 175px;
}
.odc-statuspopover-content {
min-height: 175px;
display: flex;
flex-direction: column;
pre {
flex-grow: 1;
}
}

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Tested it locally and works fine.
Add a small UI improvement idea, if you like it we can go with it, otherwise this lgtm.

@karthikjeeyar karthikjeeyar force-pushed the feat-taskrun-list branch 2 times, most recently from aa4ef52 to 58c450c Compare November 11, 2020 17:59
@karthikjeeyar
Copy link
Contributor Author

Thanks @jerolimov, definitely it looks much better now :) I have made the suggested changes.

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Cool, tested it locally, works fine

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2020
@karthikjeeyar
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

Copy link

@bgliwa01 bgliwa01 left a comment

Choose a reason for hiding this comment

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

lgtm!

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@andrewballantyne
Copy link
Contributor

New pipeline-plugin package was established, we'll need you to cherry pick this over onto that. Hopefully it is straight forward as the directory structure should be relatively unchanged.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@andrewballantyne
Copy link
Contributor

/hold

It tested all last night, which is a waste of resources. Unhold after the rebase.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 13, 2020
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 16, 2020
@karthikjeeyar
Copy link
Contributor Author

Rebased the PR
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2020
@karthikjeeyar
Copy link
Contributor Author

/retest

2 similar comments
@karthikjeeyar
Copy link
Contributor Author

/retest

@karthikjeeyar
Copy link
Contributor Author

/retest

@karthikjeeyar
Copy link
Contributor Author

Checks are still failing, putting on hold for now. Will remove it after sometime.
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2020
@karthikjeeyar
Copy link
Contributor Author

karthikjeeyar commented Nov 16, 2020

Moved all the newly created files to the new pipelines-plugin package.
cc: @jerolimov
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2020
Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Still works fine 👍

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgliwa01, jerolimov, karthikjeeyar, rohitkrai03

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

@jerolimov
Copy link
Member

/retest

@openshift-merge-robot openshift-merge-robot merged commit 14adef3 into openshift:master Nov 16, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants