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: inconsistent timezone handling in daily allocation aggregation #9888

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

amandavialva01
Copy link
Contributor

@amandavialva01 amandavialva01 commented Sep 3, 2024

Ticket

CM-440

Description

Our Historical Usage page fetches allocations by date in UTC time specifically, and we store allocations without a timezone, which converts the input time to UTC before storing the timestamp without the timezone internally.

However, fetching timestamps with no timezone and then casting to timestamptz (with a timezone) only adds a timezone to the stored time stamp, doing no internal conversion from UTC to the corresponding time in the user's set timezone. (so ('2024-10-02 01:30'::timestamp)::timestamptz = 2024-10-02 01:30 <user_set_timezone>, which isn't what we want.

With the changes in this PR, we keep consistent with how allocations are stored and continue to handle them in the resource_aggregates table without casting to a timestamptz.

Test Plan

CI passes (automated testing).

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.72%. Comparing base (3a91552) to head (ee4d92c).
Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9888      +/-   ##
==========================================
+ Coverage   54.71%   54.72%   +0.01%     
==========================================
  Files        1261     1261              
  Lines      155984   155984              
  Branches     3589     3590       +1     
==========================================
+ Hits        85348    85365      +17     
+ Misses      70504    70487      -17     
  Partials      132      132              
Flag Coverage Δ
backend 45.20% <100.00%> (+0.03%) ⬆️
harness 72.60% <ø> (ø)
web 54.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
master/internal/db/postgres_cluster.go 50.54% <100.00%> (+32.96%) ⬆️

... and 6 files with indirect coverage changes

Copy link

netlify bot commented Sep 3, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit ee4d92c
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66d73cc985845c00082e4378

@amandavialva01 amandavialva01 force-pushed the amanda/fixResourceAggregationCollection branch from a27f031 to cead271 Compare September 3, 2024 16:33
@amandavialva01 amandavialva01 force-pushed the amanda/fixResourceAggregationCollection branch from cead271 to ee4d92c Compare September 3, 2024 16:43
Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

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

LGTM

@amandavialva01 amandavialva01 merged commit d5d647a into main Sep 3, 2024
82 of 95 checks passed
@amandavialva01 amandavialva01 deleted the amanda/fixResourceAggregationCollection branch September 3, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants