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: Use ISO 8601 date format #4141

Merged
merged 2 commits into from
May 1, 2024
Merged

Conversation

jdmulloy
Copy link
Contributor

what

  • Change output of timestamps to be ISO 8601 standard format (YYYY-MM-DD) instead of (DD-MM-YYYY)

why

  • DD-MM-YYYY is confusing to people who are used to MM-DD-YYYY and it doesn't sort correctly.

tests

  • It's a simple change and I don't have time to run tests

references

@jdmulloy jdmulloy requested review from a team as code owners January 11, 2024 16:34
@jdmulloy jdmulloy requested review from jamengual, lukemassa and nitrocode and removed request for a team January 11, 2024 16:34
@github-actions github-actions bot added the go Pull requests that update Go code label Jan 11, 2024
@jamengual
Copy link
Contributor

people rely on this since this is standard for a while in atlantis, we just can't change it because is confusing for some people.

nitrocode
nitrocode previously approved these changes Jan 11, 2024
@nitrocode
Copy link
Member

@jamengual what if it was behind a flag?

@nitrocode nitrocode dismissed their stale review January 11, 2024 20:17

dismiss due to pepe's comment

@jdmulloy
Copy link
Contributor Author

@jamengual what if it was behind a flag?

I would be fine with that. I looked for a config flag at first and didn't find one. I don't quite know how to do that. I don't know golang unfortunately.

@chenrui333
Copy link
Member

yeah, it should be behind a flag, and we can deprecate/remove the old timestamp format.

@jamengual
Copy link
Contributor

jamengual commented Jan 11, 2024 via email

@nitrocode
Copy link
Member

What's so bad about adding more flags?

Is the fear that we'll add a flag and not deprecate it later when it's chosen as the new path forward (breaking change) ?

If so, then I'd suggest we add a flag and time-box it. If the flag is opt-in and it doesn't cause issues and it's better practice, then in a follow up release, we should remove the flag and set the feature as the default.

What do you folks think?

@lukemassa
Copy link
Contributor

What's so bad about adding more flags?

The more flags we add the harder it is for users to know what combination they need, the longer and more confusion our documentation becomes, and the larger the space of possible configuration combinations, many of which are untested.

Additionally, adding flags means we have to support that behavior unless we explicitly deprecate it, which takes a lot of time and effort. I recently put several hours into #2992, which of course is necessary for any large project, but adds up. There's very little pressure on removing flags/features, so it's very easy for them to grow without bound.

Certainly configurability has benefits, and any one option (including this one) might be perfectly reasonable, I'm just trying to call out the tradeoff, especially cumulatively.

@lukemassa
Copy link
Contributor

lukemassa commented Jan 18, 2024

For what it's worth, in this case I would argue to unconditionally change to YYYY-MM-DD. It conforms more to standards and is less ambiguous.

As for it affecting users, if I expected "17-01-2023" and saw "2023-01-17", even if I really preferred "17-01-2023", I feel like I would understand quickly what happened. Even if it were being parsed by some code (hopefully not!), the failure would be most likely obvious. If we were going from one arbitrary format to another I would be more sympathetic, but we're going towards a more standard ones, and I'd imagine our users would sympathize.

(As an aside, I'm actually not sure if I've ever seen DD-MM-YYYY anywhere before. Certainly DD/MM/YYYY or MM/DD/YYYY, but with the dashes it looks very strange to me)

@jamengual
Copy link
Contributor

jamengual commented Jan 18, 2024 via email

@lukemassa
Copy link
Contributor

@jdmulloy I took the liberty of fixing the failed unit test, very straightforward

@GenPage GenPage added this to the v0.28.0 milestone Jan 20, 2024
@GenPage GenPage added feature New functionality/enhancement refactoring Code refactoring that doesn't add additional functionality quick-win Is obviously something Atlantis should do and will take <4 hrs work never-stale breaking-change labels Jan 20, 2024
@GenPage
Copy link
Member

GenPage commented Jan 20, 2024

I agree with @lukemassa; we should add flags for something like that and conform to standards. In this case, we can change and highlight a breaking change as part of a minor release.

@chenrui333
Copy link
Member

another way of thinking about the flags, we can RFC introducing some prefix (serving some flag grouping purpose so that we can prioritize the deletions when the migration is done. )

lukemassa
lukemassa previously approved these changes Jan 22, 2024
nitrocode
nitrocode previously approved these changes Feb 28, 2024
@lukemassa lukemassa force-pushed the jdmulloy/iso8601 branch from b198db1 to fa77ec6 Compare May 1, 2024 03:35
@lukemassa lukemassa merged commit 93578d8 into runatlantis:main May 1, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change feature New functionality/enhancement go Pull requests that update Go code never-stale quick-win Is obviously something Atlantis should do and will take <4 hrs work refactoring Code refactoring that doesn't add additional functionality
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

6 participants