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

[ui] Simplify times in task events #18595

Merged

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Sep 27, 2023

Resolves #18507

Task restarts (and possibly other events) use more time specificity than is needed (for example, "task restarting in 10.01934802s").

This simplifies and rounds times to simple second, minute, or hour strings where relevant.

@philrenaud
Copy link
Contributor Author

Q for @DingoEatingFuzz : Is the most appropriate place for me to handle this in the TaskEvent model, or in the TaskEvent Serializer?

There's already a

  attrs = {
    message: 'DisplayMessage',
  };

aliasing happening in the serializer which I think can probably be removed with this change, but wanted to double check.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

Ember Test Audit comparison

main 5daeec8 change
passes 1532 1533 +1
failures 1 1 0
flaky 0 0 0
duration 000ms 000ms -000ms

@DingoEatingFuzz
Copy link
Contributor

is the most appropriate place for me to handle this in the TaskEvent model, or in the TaskEvent Serializer?

Personally I see this as presentation logic vs. data logic, so I like it in the model. I could also see it as a helper (though that's also a little odd since it operates on a whole string that happens to have a duration in it, not just a duration).

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Great time to use unit tests, you love to see it!

@philrenaud philrenaud force-pushed the 18507-reduce-reported-precision-on-nomad-ui-page-for-task branch from 192eb9f to 5daeec8 Compare September 27, 2023 21:01
@philrenaud philrenaud merged commit 8590876 into main Sep 27, 2023
7 of 8 checks passed
@philrenaud philrenaud deleted the 18507-reduce-reported-precision-on-nomad-ui-page-for-task branch September 27, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce reported precision on Nomad UI page for task
3 participants