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

ui: set ui.display_timezone as public #106530

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

zachlite
Copy link
Contributor

Resolves: #106415
Epic: none
Release note (ui change): The visibility of the cluster setting ui.display_timezone has been set to public. Documentation of the cluster setting has been added. No functionality has been changed.

Resolves: cockroachdb#106415
Epic: none
Release note (ui change): The visibility of the cluster setting
`ui.display_timezone` has been set to public. Documentation
of the cluster setting has been added. No functionality
has been changed.
@zachlite zachlite requested review from a team July 10, 2023 16:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@zachlite
Copy link
Contributor Author

cc @florence-crl

@blathers-crl
Copy link

blathers-crl bot commented Jul 10, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

j82w
j82w previously requested changes Jul 10, 2023
Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @zachlite)


-- commits line 7 at r1:
I think this should remain internal until a cluster setting that is capable of handling all the timezones is created. Only supporting 2 timezones will cause confusion and feature requests for other timezones. The current setting looks like it won't be the best user experience to support all possible timezones, so it will likely result in a breaking change for a better experience. Would it not be better till timezones are fully supported?

@j82w j82w dismissed their stale review July 10, 2023 18:11

offline discussion, already publicly documented.

@zachlite
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 10, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set public = t for ui.display_timezone cluster setting
4 participants