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

Jump to bottom

mgr/dashboard: fix columns in host table with NaN Undefined #44843

Merged
merged 1 commit into from Mar 16, 2022
Merged

mgr/dashboard: fix columns in host table with NaN Undefined #44843

merged 1 commit into from Mar 16, 2022

Conversation

avanthakkar
Copy link
Contributor

@avanthakkar avanthakkar commented Jan 31, 2022 •
edited

Fixes: https://tracker.ceph.com/issues/54068
Signed-off-by: Avan Thakkar athakkar@redhat.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@avanthakkar avanthakkar added this to In progress in Dashboard via automation Jan 31, 2022
@avanthakkar avanthakkar marked this pull request as ready for review February 1, 2022 12:04
@avanthakkar avanthakkar requested a review from a team as a code owner February 1, 2022 12:04
@avanthakkar avanthakkar requested review from pereman2, Sarthak0702, nizamial09 and epuertat and removed request for a team February 1, 2022 12:04
@avanthakkar avanthakkar added the skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology label Feb 1, 2022
@nizamial09
Copy link
Member

jenkins test dashboard cephadm

pereman2
pereman2 reviewed Feb 1, 2022
View reviewed changes
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/hosts/hosts.component.ts Outdated
Comment on lines 467 to 473
hostKey['memory_total_bytes'] = !isNaN(hostKey['memory_total_kb'])
? hostKey['memory_total_kb'] * 1024
: 'N/A';
hostKey['raw_capacity'] =
!isNaN(hostKey['hdd_capacity_bytes']) && !isNaN(hostKey['flash_capacity_bytes'])
? hostKey['hdd_capacity_bytes'] + hostKey['flash_capacity_bytes']
: 'N/A';
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly, if a ternary assignment is not a one liner I would prefer a usual if statement.

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 avoided writing if else block twice)

Dashboard automation moved this from In progress to Reviewer approved Feb 2, 2022
@avanthakkar
Copy link
Contributor Author

jenkins test make check

@avanthakkar
Copy link
Contributor Author

jenkins test make check

@avanthakkar
Copy link
Contributor Author

jenkins test dashboard

@epuertat epuertat moved this from Reviewer approved to Ready-to-merge in Dashboard Feb 4, 2022
Dashboard automation moved this from Ready-to-merge to Review in progress Feb 4, 2022
epuertat
epuertat previously requested changes Feb 4, 2022
View reviewed changes
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/hosts/hosts.component.ts Outdated
Comment on lines 467 to 468
hostKey['memory_total_bytes'] = !isNaN(hostKey['memory_total_kb'])
? hostKey['memory_total_kb'] * 1024
Copy link
Member

Choose a reason for hiding this comment

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

Why do we care about presentation stuff in the logic layer? NaN or "N/A" is not relevant at this stage.

IMHO that should be left for the template and pipes are good at this. We even have a pipe empty that basically converts null and undefined to "-". If we need to deal with NaN let's add that check to that pipe and keep a consistent behaviour ("-" for null/undefined in some places, but "N/A" in others). That's the good thing of reusing helpers.

And going one step further: if this NaN/null/undefined seems to be a recurrent issue (I remember at least another 2 recent PRs dealing with it), why don't we append the empty pipe by default to the cd-datatable cells? That way we'll avoid having to manual check for this in every single piece of data that we display.

Additionally, is it normal to get a NaN here? Shouldn't we get a null or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we care about presentation stuff in the logic layer? NaN or "N/A" is not relevant at this stage.

IMHO that should be left for the template and pipes are good at this. We even have a pipe empty that basically converts null and undefined to "-". If we need to deal with NaN let's add that check to that pipe and keep a consistent behaviour ("-" for null/undefined in some places, but "N/A" in others). That's the good thing of reusing helpers.

And going one step further: if this NaN/null/undefined seems to be a recurrent issue (I remember at least another 2 recent PRs dealing with it), why don't we append the empty pipe by default to the cd-datatable cells? That way we'll avoid having to manual check for this in every single piece of data that we display.

Additionally, is it normal to get a NaN here? Shouldn't we get a null or undefined?

Yes but if we do arithmetic operation with a number and those undefined values it'll NaN.

@epuertat
Copy link
Member

epuertat commented Feb 4, 2022

jenkins test make check

@avanthakkar avanthakkar force-pushed the fix-Nan-undefined-host-columns branch from 0d9d804 to 41ca50c Compare March 7, 2022 16:14
@avanthakkar
Copy link
Contributor Author

jenkins test make check

@avanthakkar
Copy link
Contributor Author

jenkins test windows

1 similar comment
@avanthakkar
Copy link
Contributor Author

jenkins test windows

@epuertat
Copy link
Member

jenkins test windows

@epuertat epuertat moved this from Review in progress to Ready-to-merge in Dashboard Mar 16, 2022
Dashboard automation moved this from Ready-to-merge to Reviewer approved Mar 16, 2022
@avanthakkar avanthakkar moved this from Reviewer approved to needs-qa in Dashboard Mar 16, 2022
@avanthakkar avanthakkar moved this from needs-qa to Ready-to-merge in Dashboard Mar 16, 2022
@epuertat epuertat merged commit f1ef098 into ceph:master Mar 16, 2022
13 checks passed
@epuertat epuertat deleted the fix-Nan-undefined-host-columns branch March 16, 2022 13:12
Dashboard automation moved this from Ready-to-merge to Done Mar 16, 2022
@epuertat epuertat mentioned this pull request May 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pereman2 pereman2 pereman2 approved these changes

@nizamial09 nizamial09 nizamial09 approved these changes

@Sarthak0702 Sarthak0702 Awaiting requested review from Sarthak0702 Sarthak0702 was automatically assigned from ceph/dashboard

@epuertat epuertat Awaiting requested review from epuertat

Assignees
No one assigned
Labels
dashboard pybind skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology
Projects
Ceph-Dashboard
Archived in project
Dashboard
  
Done
Milestone
No milestone
4 participants
@avanthakkar @nizamial09 @epuertat @pereman2