Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Fix: use system time instead of UTC in ResourceManager.share #4910

Merged
merged 2 commits into from
Nov 19, 2019

Conversation

mfranciszkiewicz
Copy link
Contributor

No description provided.

Copy link
Contributor

@maaktweluit maaktweluit left a comment

Choose a reason for hiding this comment

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

LGTM!

One small comment and a q:
Shouldnt this be fixed in simple-transfer instead?

@@ -48,7 +48,8 @@ def share(
# Missing / invalid timeouts are validated by the client.
# Don't pre-check it here and set the timeout to max int.
timeout = client_options.timeout or sys.maxsize
now = get_timestamp_utc()
# Use system time like simple-transfer does
now = datetime.now().timestamp()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this be default_now like in the database?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default_now returns a UTC datetime object

@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #4910 into b0.22 will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            b0.22    #4910      +/-   ##
==========================================
+ Coverage   89.21%   89.23%   +0.02%     
==========================================
  Files         233      233              
  Lines       21419    21419              
==========================================
+ Hits        19108    19113       +5     
+ Misses       2311     2306       -5

@mfranciszkiewicz
Copy link
Contributor Author

@maaktweluit We'd have to release a new version of simple-transfer and afaict that wasn't planned

@mfranciszkiewicz mfranciszkiewicz merged commit b8e22b7 into b0.22 Nov 19, 2019
@mfranciszkiewicz mfranciszkiewicz deleted the resource_manager_sys_time branch November 19, 2019 15:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants