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

Bug 1902003: Clarification of Jobs completions column data when sorting #7760

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Jan 7, 2021

https://bugzilla.redhat.com/show_bug.cgi?id=1902003 was opened regarding “Incorrect” sorting of the Job Completions column. The column data is sorting correctly on desired {completions} but the content is presented as {succeeded} of {completions} (eg 1 of 1) . When sorted descending order, the following visually looks incorrect listed as, 2 of 20, 3 of 8, 1 of 1.
To alleviate this confusion I suggest we include stack the Desired {completions} above Succeeded {job.status.succeeded || 0}.

Screen Shot 2021-01-06 at 3 36 41 PM

Screen Shot 2021-01-06 at 3 32 54 PM

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jan 7, 2021
@sg00dwin sg00dwin changed the title Clarification of Jobs completions column data when sorting Bug 1902003 : Clarification of Jobs completions column data when sorting Jan 7, 2021
@sg00dwin sg00dwin force-pushed the job-completions-confusion-with-sort branch from 981a870 to 6f46853 Compare January 8, 2021 18:51
Copy link
Contributor

@rebeccaalpert rebeccaalpert left a comment

Choose a reason for hiding this comment

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

Thanks for making that change.

@sg00dwin sg00dwin force-pushed the job-completions-confusion-with-sort branch from 6f46853 to c231144 Compare January 8, 2021 20:20
@spadgett
Copy link
Member

spadgett commented Jan 8, 2021

If we're going to do this, would it be better to simply make them separate columns? Then you could sort either by successful or total.

@sg00dwin
Copy link
Member Author

If we're going to do this, would it be better to simply make them separate columns? Then you could sort either by successful or total.

I assume it would depend on how valuable it is to be able to sort by most currently succeeded is vs the additional space constraints. Personally I think the stacking of the two works well from a content presentation scanning perspective.

Screen Shot 2021-01-11 at 10 53 28 AM

@spadgett
Copy link
Member

cc @beanh66 for UX input

@sg00dwin sg00dwin changed the title Bug 1902003 : Clarification of Jobs completions column data when sorting Bug 1902003: Clarification of Jobs completions column data when sorting Jan 15, 2021
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low 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 Jan 15, 2021
@openshift-ci-robot
Copy link
Contributor

@sg00dwin: This pull request references Bugzilla bug 1902003, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

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

In response to this:

Bug 1902003: Clarification of Jobs completions column data when sorting

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.

@cshinn
Copy link

cshinn commented Jan 20, 2021

Does every column need to be sortable? The initial format is concise and pretty clear and takes less space than separate columns would

@sg00dwin
Copy link
Member Author

Does every column need to be sortable? The initial format is concise and pretty clear and takes less space than separate columns would

Columns don't necessarily need to be sortable, though it seems like a reasonable data to sort by.

@spadgett
Copy link
Member

I was wondering if we shouldn't just change the sort to use the first number (succeeded) instead of desired

@beanh66
Copy link
Member

beanh66 commented Jan 20, 2021

That was my initial thought as well @spadgett. The column header even makes me think that's what is being displayed anyways since Completed may imply "successfully completed."

For what it's worth, for Pods we have a column called "Ready" and then we have x of y and we seem to sort on the first number. Machine sets on the other hand seem to have the same problem as jobs, where we sorting on the second number and maybe should change to sort on the first.

@sg00dwin sg00dwin force-pushed the job-completions-confusion-with-sort branch from c231144 to df5ee30 Compare January 21, 2021 15:59
@sg00dwin
Copy link
Member Author

I have updated this pr to sort by Succeeded Completions and reverted to display the original "x of y".
Screen Shot 2021-01-21 at 10 36 50 AM

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@sg00dwin Thanks, can you look at updating the MachineSet sort as well?

frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/job.tsx Outdated Show resolved Hide resolved
@sg00dwin sg00dwin force-pushed the job-completions-confusion-with-sort branch 2 times, most recently from 5a410f1 to c0cf2a3 Compare January 25, 2021 15:33
@sg00dwin
Copy link
Member Author

/retest

1 similar comment
@sg00dwin
Copy link
Member Author

/retest

@spadgett
Copy link
Member

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

@openshift-ci-robot
Copy link
Contributor

@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin

In response to this:

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

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.

@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 1, 2021

/retest

additional change for consistency, sort MachineSet Machines by ready Replicas instead of desired Replicas
Fixes bug https://bugzilla.redhat.com/show_bug.cgi?id=1902003
@sg00dwin sg00dwin force-pushed the job-completions-confusion-with-sort branch from c0cf2a3 to c012085 Compare February 8, 2021 17:17
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 8, 2021

@spadgett pr updated to include sorting MachineSet Machines by ready vs desired

@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 9, 2021

/retest

1 similar comment
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 9, 2021

/retest

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@spadgett spadgett added this to the v4.8 milestone Feb 9, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rebeccaalpert, sg00dwin, spadgett

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 Feb 9, 2021
@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit fe97944 into openshift:master Feb 10, 2021
@openshift-ci-robot
Copy link
Contributor

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

Bugzilla bug 1902003 has been moved to the MODIFIED state.

In response to this:

Bug 1902003: Clarification of Jobs completions column data when sorting

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.

@sg00dwin sg00dwin deleted the job-completions-confusion-with-sort branch February 10, 2021 21:38
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. bugzilla/severity-low Referenced Bugzilla bug's severity is low 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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants