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(dataset): Ensure DATETIME FORMAT is ISO 8601 compliant #26017

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Nov 17, 2023

SUMMARY

The dataset DATETIME FORMAT should be ISO 8601 compliant for lexicographical ordering purposes. This is mentioned throughout the code however the placeholder text for the dataset editor was %Y/%m/%d which is non-compliant and likely lead people astray which resulted in #24113.

#25510 reverts the validation logic, though this PR also ensures that the underlying placeholder text is changed to %Y-%m-%d per the ISO 8601 standard.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Updated unit tests.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

cc: @jfrag1

@john-bodley john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Nov 17, 2023
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Blocking this PR until we resolve if it constitutes a breaking change or not.

@michael-s-molina michael-s-molina added the hold! On hold label Nov 17, 2023
@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Nov 20, 2023
@john-bodley john-bodley added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jan 3, 2024
@michael-s-molina michael-s-molina removed the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jan 16, 2024
@john-bodley
Copy link
Member Author

@michael-s-molina I gather because we're now in a breaking window we could merge this PR irrespective of whether it constitutes a breaking change or not.

@john-bodley john-bodley added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jan 18, 2024
@michael-s-molina
Copy link
Member

michael-s-molina commented Jan 19, 2024

@michael-s-molina I gather because we're now in a breaking window we could merge this PR irrespective of whether it constitutes a breaking change or not.

@john-bodley Only breaking changes that were approved by lazy consensus can be merged as someone might have objections during the lazy consensus period. I think the correct process here is to add a card to Punted to 5.0 column so we don't miss this again in the next breaking window.

@michael-s-molina michael-s-molina removed the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jan 19, 2024
@john-bodley john-bodley removed the hold! On hold label Jan 23, 2024
@john-bodley
Copy link
Member Author

Closing in favor of #25510 which incorporated these changes.

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.

2 participants