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

fix duration for workload #173

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

warjiang
Copy link
Contributor

@warjiang warjiang commented Jan 20, 2025

What type of PR is this?
/kind bug

What this PR does / why we need it:
The duration of workload is hard coded in the frontend code, just fix that.
Which issue(s) this PR fixes:
Fixes #169

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

- fix the duration calculation for workload 

@karmada-bot karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 20, 2025
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2025
@warjiang
Copy link
Contributor Author

/assign @samzong

Comment on lines 5 to 6
const startDate = dayjs(start);
const endDate = dayjs(); // 当前时间
Copy link
Member

Choose a reason for hiding this comment

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

The backend usually returns the server time, which should be UTC by default, and it may be necessary to handle the time zone differences; here, is it the browser time being obtained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a good question 🤔 , and dayjs(which we used in our project) use browser timezone in default, and if the timezone differs from local(here refer our personal computer) and server(which means the karmada-dashboard-api ), the calculation of duration may be wrong

Copy link
Member

Choose a reason for hiding this comment

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

dayjs support plugin utc and timezone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dayjs support plugin utc and timezone.

yeah, but that's not key problem, key problem is the difference between client time for different regions. In fact the duration for workload is an absolute number, should be different across regions.

Copy link
Member

Choose a reason for hiding this comment

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

maybe just convert it to the current time zone of the browser.

No need to consider multiple time zones. like this.

import dayjs from 'dayjs';
import utc from 'dayjs/plugin/utc';
import timezone from 'dayjs/plugin/timezone';

dayjs.extend(utc);
dayjs.extend(timezone);

export function calculateDuration(start: string | undefined) {
  if (!start) return '-';
  // Convert UTC server time to browser's local timezone
  const startDate = dayjs.utc(start).local();
  const endDate = dayjs();
  const duration = dayjs.duration(endDate.diff(startDate));
  return duration.humanize();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, that's a good idea. Just now I still work on getting timezone from server and set defaut timezone for dayjs in frontend. Converting start(server time) to client time is more simple than what i did just now, great~

@RainbowMango
Copy link
Member

I think we can squash the commits before moving this forward.

@warjiang warjiang force-pushed the fix/workload-duration branch from 2d161f6 to a2e0189 Compare January 21, 2025 13:15
@warjiang
Copy link
Contributor Author

I think we can squash the commits before moving this forward.

thanks for your kindlly remind, 😄

@warjiang warjiang force-pushed the fix/workload-duration branch from a2e0189 to 4c6aa68 Compare January 21, 2025 13:22
@warjiang
Copy link
Contributor Author

@samzong plz check it again~

Copy link
Member

@samzong samzong left a comment

Choose a reason for hiding this comment

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

great!

@samzong
Copy link
Member

samzong commented Jan 21, 2025

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2025
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: samzong

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2025
@karmada-bot karmada-bot merged commit d4794ad into karmada-io:main Jan 21, 2025
7 checks passed
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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

in deployment detail page, 持续时间字段计算有误
4 participants