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

Revert "lib/, src/: Use local time for human-readable dates" #1207

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Feb 14, 2025

This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since Epoch. We don't have enough precision to translate it into local time. Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since printing a date without time-zone information means that the date is a local date, but what we're printing is a UTC date. A future patch should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20
Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430
Link: https://lists.iana.org/hyperkitty/list/[email protected]/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/
Link: #1202
Link: #1057
Link: #939
Link: #1058
Link: #1059 (comment)
Link: #952
Link: #942
Reported-by: @zeha
Reported-by: @kenion
Reported-by: @alejandro-colomar
Reported-by: @jubalh
Reported-by: @leegarrett
Cc: @eggert
Cc: @timparenti
Cc: @ikerexxe
Cc: @hallyn
Cc: @BrianInglis


Here goes the first round of changes. This reverts back to the old behavior.


Revisions:

v1b
  • Rebase
$ git range-diff alx/master..gh/t shadow/master..t
1:  0f13c140 = 1:  e49f4a57 Revert "lib/, src/: Use local time for human-readable dates"
v1c
$ git range-diff master gh/t t 
1:  e49f4a57 ! 1:  a0c5dbd6 Revert "lib/, src/: Use local time for human-readable dates"
    @@ Commit message
         Reported-by: Lee Garrett <[email protected]>
         Cc: Paul Eggert <[email protected]>
         Cc: Tim Parenti <[email protected]>
    -    Cc: Iker Pedrosa <[email protected]>
         Cc: "Serge E. Hallyn" <[email protected]>
         Cc: Brian Inglis <[email protected]>
    +    Reviewed-by: Iker Pedrosa <[email protected]>
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## lib/time/day_to_str.h ##

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 14, 2025

Everyone affected by all the date-related bugs, can you please explicitly state if this change is good, bad, or neutral to your issues?

@zeha
Copy link
Contributor

zeha commented Feb 15, 2025

I've applied this revert on top of 4.17.2 and the problem described in #1202 is still there.

@alejandro-colomar
Copy link
Collaborator Author

I've applied this revert on top of 4.17.2 and the problem described in #1202 is still there.

Okay, I'll check that. Thanks for testing!

@alejandro-colomar alejandro-colomar marked this pull request as draft February 15, 2025 23:15
@alejandro-colomar
Copy link
Collaborator Author

@zeha

From what you've said in the bug report and here, this doesn't seem to affect neither positively nor negatively to your case. Since the change is technically good, and doesn't seem to make things worse, I'll re-open the PR for review.

Let's continue with the plan.

At least, this should allow you to see the real date that is being stored on file without having to look at the raw number in the shadow file.

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking care of it.

This reverts commit 3f5b4b5.

The dates are stored as UTC, and are stored as a number of days since
Epoch.  We don't have enough precision to translate it into local time.
Using local time has caused endless issues in users.

This patch is not enough for fixing this issue completely, since
printing a date without time-zone information means that the date is a
local date, but what we're printing is a UTC date.  A future patch
should add time-zone information to the date.

For now, let's revert this change that has caused so many issues.

Fixes: 3f5b4b5 (2024-08-01; "lib/, src/: Use local time for human-readable dates")
Link: <https://github.com/ansible/ansible/blob/devel/test/integration/targets/user/tasks/test_expires.yml#L2-L20>
Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1095430>
Link: <https://lists.iana.org/hyperkitty/list/[email protected]/message/ENE5IFV3GAH6WK22UJ6YU57D6TQINSP5/>
Link: <shadow-maint#1202>
Link: <shadow-maint#1057>
Link: <shadow-maint#939>
Link: <shadow-maint#1058>
Link: <shadow-maint#1059 (comment)>
Link: <shadow-maint#952>
Link: <shadow-maint#942>
Reported-by: Chris Hofstaedtler <[email protected]>
Reported-by: Gus Kenion <https://github.com/kenion>
Reported-by: Alejandro Colomar <[email protected]>
Reported-by: Michael Vetter <[email protected]>
Reported-by: Lee Garrett <[email protected]>
Cc: Paul Eggert <[email protected]>
Cc: Tim Parenti <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Brian Inglis <[email protected]>
Reviewed-by: Iker Pedrosa <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar alejandro-colomar merged commit dfb2fa3 into shadow-maint:master Feb 25, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants