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

Switch to zoneinfo from pytz #7645

Merged
merged 9 commits into from
Dec 22, 2024
Merged

Conversation

matmair
Copy link
Member

@matmair matmair commented Jul 13, 2024

We target 3.9 which has zoneinfo for timezone info instead of pytz.

Unverified

This user has not yet uploaded their public signing key.
@matmair matmair added security Relates to a security issue refactor labels Jul 13, 2024
@matmair matmair added this to the 0.16.0 milestone Jul 13, 2024
@matmair matmair self-assigned this Jul 13, 2024
@matmair matmair requested a review from SchrodingersGat as a code owner July 13, 2024 21:25
Copy link

netlify bot commented Jul 13, 2024

Deploy Preview for inventree-web-pui-preview ready!

Name Link
🔨 Latest commit 309e323
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/6763334670111500084bb1e4
😎 Deploy Preview https://deploy-preview-7645--inventree-web-pui-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 86 (no change from production)
Best Practices: 100 (no change from production)
SEO: 78 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.33%. Comparing base (6634bc5) to head (309e323).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
src/backend/InvenTree/InvenTree/helpers.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7645      +/-   ##
==========================================
+ Coverage   85.24%   85.33%   +0.08%     
==========================================
  Files        1175     1175              
  Lines       52424    52430       +6     
  Branches     2075     2078       +3     
==========================================
+ Hits        44691    44740      +49     
+ Misses       7226     7180      -46     
- Partials      507      510       +3     
Flag Coverage Δ
backend 86.96% <80.00%> (ø)
migrations 43.18% <60.00%> (+<0.01%) ⬆️
pui 69.47% <ø> (+0.44%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SchrodingersGat
Copy link
Member

Nice. Should we also remove pytz from the requirements?

@matmair
Copy link
Member Author

matmair commented Jul 14, 2024

@SchrodingersGat sadly not, some of our dependencies still depend on it

@matmair matmair marked this pull request as draft July 17, 2024 16:34
@matmair
Copy link
Member Author

matmair commented Jul 17, 2024

I need to figure out why CI works on my machine but not on GH

@matmair
Copy link
Member Author

matmair commented Jul 30, 2024

This is more complex than initially thought. https://blog.ganssle.io/articles/2018/03/pytz-fastest-footgun.html
I am amazed we have not had problems with pytz before.

@matmair matmair modified the milestones: 0.16.0, 1.0.0 Jul 30, 2024
@SchrodingersGat
Copy link
Member

Wow, what a deep dive!

@SchrodingersGat
Copy link
Member

@matmair it would be great to get this one closed out if you have some time to assign to it :)

@matmair
Copy link
Member Author

matmair commented Nov 20, 2024

@SchrodingersGat this is more a skill not a time issue ;-) I can look into it after sso and selection lists

@SchrodingersGat
Copy link
Member

Ok, LMK if you are banging your head against the wall on it!

@matmair matmair added the full-run Always do a full QC CI run label Dec 17, 2024
@matmair
Copy link
Member Author

matmair commented Dec 18, 2024

@SchrodingersGat can you take a look at this? Especially the change in the test assertations. Maybe I am missing something but the previous assertations seem strangely off by a few minutes.

@matmair matmair marked this pull request as ready for review December 18, 2024 02:29
@1337joe
Copy link
Contributor

1337joe commented Dec 21, 2024

To add some specific discussions of this to add to your link above:
https://stackoverflow.com/questions/27531718/datetime-timezone-conversion-using-pytz
https://stackoverflow.com/questions/60996205/python-datetime-timezone-conversion-off-by-4-minutes

Long story short, using replace(tzinfo=... (what InvenTree was using) can make invalid assumptions, where localize(time) would try to correct for that. The old test values were the incorrect values that pytz would produce for those zone conversions.

For what it's worth, your updated test values are correct from my manual checking on: https://dateful.com/time-zone-converter

@matmair matmair added breaking Indicates a major update or change which breaks compatibility and removed full-run Always do a full QC CI run labels Dec 22, 2024
@matmair
Copy link
Member Author

matmair commented Dec 22, 2024

@SchrodingersGat ready for review and merge; I have marked it as breaking as a precaution, I do not think it should break anything

@SchrodingersGat
Copy link
Member

@matmair seems like a minimal change overall, in the end. Thank you for looking into this one.

@SchrodingersGat SchrodingersGat merged commit aa90516 into inventree:master Dec 22, 2024
45 checks passed
@SchrodingersGat SchrodingersGat deleted the switch-pytz branch December 22, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Indicates a major update or change which breaks compatibility refactor security Relates to a security issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants