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.

Sign up for GitHub

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

Jump to bottom

Bug 1873230: show message for retrying in conclusion if there are failed tasks #6465

Merged
merged 1 commit into from Sep 3, 2020
Merged

Bug 1873230: show message for retrying in conclusion if there are failed tasks #6465

merged 1 commit into from Sep 3, 2020

Conversation

nemesis09
Copy link
Contributor

Fixes:
https://issues.redhat.com/browse/ODC-4638

Analysis / Root cause:
Currently when one or more steps have not been accomplished successfully, the Quick Start displays the positive conclusion message provided

Solution Description:
If one or more steps have not been accomplished successfully, on the last page of the Quick Start

  1. Post this generic message rather than the conclusion "One or more verifications did not pass during this quick start. Revisit the tasks or the help links, and then try again."
  2. If there's a follow on tour, do not link to it

Screens:
Before
Screenshot from 2020-08-27 22-21-51
Screenshot from 2020-08-27 22-22-49

After
Screenshot from 2020-08-27 21-42-27
Screenshot from 2020-08-27 21-42-54

Test Coverage:
Screenshot from 2020-08-27 22-19-45

Browser Conformance:

  • Firefox
  • Chrome
  • Safari
  • Edge

@nemesis09 nemesis09 changed the title show message for retrying in conclusion if there are failed tasks Bug 1873230: show message for retrying in conclusion if there are failed tasks Aug 27, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 27, 2020
@openshift-ci-robot
Copy link
Contributor

@nemesis09: This pull request references Bugzilla bug 1873230, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1873230: show message for retrying in conclusion if there are failed tasks

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nemesis09
Copy link
Contributor Author

cc @serenamarie125

rohitkrai03
rohitkrai03 suggested changes Aug 28, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/controller/QuickStartConclusion.tsx Outdated
<SyncMarkdownView content={conclusion} />
{nextQuickStart && (
{tasks.map((task, index) => {
if (allTaskStatuses[index] === QuickStartTaskStatus.FAILED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to check the status for each task like this. Just check if allTaskStatuses has at least one failed status in the array. If yes then show error message else show conclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought i would avoid introducing a new loop and use the existing one

Copy link
Contributor Author

@nemesis09 nemesis09 Aug 28, 2020 •
edited

Choose a reason for hiding this comment

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

updated the check to const hasFailedTask = allTaskStatuses.includes(QuickStartTaskStatus.FAILED);

@nemesis09 nemesis09 force-pushed the fix-quick-start-conclusion branch 3 times, most recently from 5b42121 to 9e856d6 Compare August 28, 2020 13:09
abhinandan13jan
abhinandan13jan reviewed Sep 3, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/controller/QuickStartConclusion.tsx Outdated
{nextQuickStart && (
<SyncMarkdownView
content={
!hasFailedTask
Copy link
Contributor

Choose a reason for hiding this comment

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

@nemesis09 can you please reverse it. It's better to use positivity check, remove the !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

jerolimov
jerolimov approved these changes Sep 3, 2020
View reviewed 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.

Tested it locally, lgtm. All other good feedback is already addressed 👍

@jerolimov
Copy link
Member

/lgtm
/assign @rohitkrai03
/retest

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

ran it locally and it works fine
/lgtm

rohitkrai03
rohitkrai03 approved these changes Sep 3, 2020
View reviewed changes
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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, jerolimov, nemesis09, 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2020
@openshift-merge-robot openshift-merge-robot merged commit ff27905 into openshift:master Sep 3, 2020
@openshift-ci-robot
Copy link
Contributor

@nemesis09: All pull requests linked via external trackers have merged:

Bugzilla bug 1873230 has been moved to the MODIFIED state.

In response to this:

Bug 1873230: show message for retrying in conclusion if there are failed tasks

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.6 milestone Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinandan13jan abhinandan13jan abhinandan13jan left review comments

@jerolimov jerolimov jerolimov approved these changes

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

Assignees

@jerolimov jerolimov

@rohitkrai03 rohitkrai03

@abhinandan13jan abhinandan13jan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.6
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nemesis09 @openshift-ci-robot @jerolimov @abhinandan13jan @rohitkrai03 @spadgett @openshift-merge-robot