Skip to content

Fix date_done timezone issue#8385

Merged
Nusnus merged 21 commits into
celery:mainfrom
FKgk:fix-date-done
Sep 1, 2024
Merged

Fix date_done timezone issue#8385
Nusnus merged 21 commits into
celery:mainfrom
FKgk:fix-date-done

Conversation

@FKgk

@FKgk FKgk commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

This PR fixes #4842 with my best friend @CodeSik.
celery_taskmeta.date_done was being saved via utcnow(), but we suggest changing it to app.now() to apply the value set in app.conf.timezone.

@Nusnus

Nusnus commented Jul 22, 2023

Copy link
Copy Markdown
Member

Thank you for the fix!
Is it possible to add some unit tests to verify the change?

@CodeSik

CodeSik commented Jul 22, 2023

Copy link
Copy Markdown
Contributor

Ok. We'll fix and add unit tests over the next week.

@Nusnus Nusnus self-requested a review July 22, 2023 21:46
@auvipy auvipy self-requested a review July 24, 2023 07:16
auvipy
auvipy previously requested changes Jul 24, 2023
View reviewed changes
Comment thread celery/backends/base.py
@auvipy

auvipy commented Nov 8, 2023

Copy link
Copy Markdown
Member

new tests?

@cclauss cclauss mentioned this pull request Jan 5, 2024
Closed
4 tasks
@Nusnus

Nusnus commented Jan 12, 2024

Copy link
Copy Markdown
Member

@FKgk @CodeSik Can you check up on the PR please?

Thank you!

@Nusnus Nusnus mentioned this pull request Jan 14, 2024
Merged
@Nusnus

Nusnus commented Jan 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member 
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

celery/backends/base.py:12:1: F401 'datetime.datetime' imported but unused
celery/backends/base.py:12:1: F401 'datetime.timezone' imported but unused

@Nusnus

Nusnus commented Jan 15, 2024

Copy link
Copy Markdown
Member

Also please notice "This branch cannot be rebased due to conflicts”.

@codecov

codecov Bot commented Jan 15, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.05%. Comparing base (f96a431) to head (642619b).
Report is 117 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8385   +/-   ##
=======================================
  Coverage   78.05%   78.05%           
=======================================
  Files         150      150           
  Lines       18791    18791           
  Branches     3213     3213           
=======================================
+ Hits        14667    14668    +1     
  Misses       3821     3821           
+ Partials      303      302    -1
Flag Coverage Δ
unittests 78.03% <100.00%> (+<0.01%) ⬆️

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.

@lianyueh

lianyueh commented Jul 9, 2024

Copy link
Copy Markdown

@auvipy @FKgk @Nusnus Could someone please resolve conversations, and merge this PR?
This is a trivial issue but matters 🥲

@auvipy

auvipy commented Jul 9, 2024

Copy link
Copy Markdown
Member

@auvipy @FKgk @Nusnus Could someone please resolve conversations, and merge this PR?
This is a trivial issue but matters 🥲

This needs test coverage

@Nusnus Nusnus marked this pull request as draft July 9, 2024 19:49
@Nusnus

Nusnus commented Jul 9, 2024

Copy link
Copy Markdown
Member

@FKgk @CodeSik Can you check up on the PR please?

Thank you!

@FKgk @CodeSik any chance for an update please?

Thanks 🙏

@FKgk

FKgk commented Aug 5, 2024

Copy link
Copy Markdown
Contributor Author

I'm sorry for the late update.
I corrected the failed test, and 1 failed due to a docker pull error.
(Error response from daemon: toomanyrequests: You have reached your pull rate limit)
Please check again 🙏

@FKgk FKgk changed the title Fix date_done timzone issue Fix date_done timezone issue Aug 5, 2024
@thedrow

thedrow commented Aug 25, 2024

Copy link
Copy Markdown
Contributor

The tests are still failing.

cclauss
cclauss reviewed Aug 25, 2024
View reviewed changes
Comment thread t/unit/tasks/test_result.py Outdated
cclauss
cclauss reviewed Aug 25, 2024
View reviewed changes
Comment thread t/unit/tasks/test_result.py Outdated
@auvipy @cclauss
Update t/unit/tasks/test_result.py
147680f 
Co-authored-by: Christian Clauss <[email protected]>
cclauss
cclauss reviewed Aug 27, 2024
View reviewed changes
Comment thread t/unit/tasks/test_result.py Outdated
@thedrow thedrow marked this pull request as ready for review September 1, 2024 13:00
thedrow
thedrow approved these changes Sep 1, 2024
View reviewed changes

@thedrow thedrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thedrow thedrow dismissed auvipy’s stale review September 1, 2024 13:01

Tests are fixed

Nusnus
Nusnus approved these changes Sep 1, 2024
View reviewed changes

@Nusnus Nusnus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM2

@Nusnus Nusnus merged commit f436f10 into celery:main Sep 1, 2024
@matthuska matthuska mentioned this pull request Nov 4, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nusnus Nusnus Nusnus approved these changes
@auvipy auvipy auvipy left review comments
+2 more reviewers
@cclauss cclauss cclauss left review comments
@thedrow thedrow thedrow approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Celery timezone use default UTC timezone ignoring settings.

7 participants

@FKgk @Nusnus @CodeSik @auvipy @lianyueh @thedrow @cclauss