feat: allow switching timezone for date rendering. Fixes #3474

[isubasinghe]

Signed-off-by: Isitha Subasinghe isitha@pipekit.io

Fixes #3474

Please do not open a pull request until you have checked ALL of these:

• Create the PR as draft .
• [] Run make pre-commit -B to fix codegen and lint problems.
• Sign-off your commits (otherwise the DCO check will fail).
• Use a conventional commit message (otherwise the commit message check will fail).
• "Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
• Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
• Github checks are green.
• Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

[alexec]

How does this work once webpack builds the app? I don’t think it has access to time zone anymore.

[isubasinghe]

@alexec it is basically a C macro is my understanding according to this: https://webpack.js.org/plugins/define-plugin/.
All it does is text replacement: process.env.DEFAULT_TZ becomes "UTC".

I can also remove this, I just thought it being configurable is nice.
Alternatively we can use dotenv files.

[JPZ13]

Hey @isubasinghe - the way the build process works for the UI is that webpack bakes these environment variables in a buildtime rather than getting them at runtime, which is annoying but common with client-side single-page-apps. To set up a way to get configurable items, there are a couple solutions:

1. Build an API endpoint that the UI can hit - this will have access to environment variables in the argo-server
2. Implement server-side rendering
3. Other stuff that I haven't seen yet

Happy to continue the thread here or chat about it on our next call

[isubasinghe]

@JPZ13 oh yeah I am aware of that aspect, correct me if I am wrong but I believe @alexec concern was that the process.env.DEFAULT_TZ was not available anymore after webpack has built the JS bundle, which I don't believe is correct. DefinePlugin should strip out process.env.DEFAULT_TZ and replace it with just 'UTC', like this static build time thing was specifically what I was aiming for with this.

I wanted to just have a configurable default timezone in the webapp that someone can configure as a part of the build process itself.

This baking step is done here through webpack itself here https://github.com/argoproj/argo-workflows/pull/10120/files#diff-17798992c0235eb019e197c72e2967fd83ba5e2703ae7672ae54c56141e90aa8R63.

I made a minimal web page here to demonstrate that process.env.DEFAULT_TZ is in fact not undefined here: https://github.com/isubasinghe/argo-workflows/blob/test-webpack/ui/src/app/testapp.tsx. If you perform a yarn build and python -m http.server you will notice that the value of process.env.DEFAULT_TZ is 'UTC' not undefined.

This value is from the previous DefinePlugin call to webpack.

I hope I made my intentions more clearer now.
Honestly perhaps reaching out to the server and querying its configured timezone is better, but for now imo this baking is good enough. Happy to change it if you disagree though.

[JPZ13]

Ah understood. It does seem like an odd developer experience to me. The user would have to create a separate docker image for the new timezone that they want to add. It seems strange to make the user do that rather than have the environment variable for DEFAULT_TZ be configurable on the argo server deployment

[isubasinghe]

@JPZ13 the intention is that this is more something we change when we(Argo Project members) would like to change the default timezone we ship with.

The timezone is still configurable from the UI. Here is the image where one may select the timezone they would like to view logs in.

I just pushed a new change which saves the selected timezone into local storage as well. A fresh workflow run results in the following output, because I detected that I had previously chosen AEDT as my timezone:

Sorry to be a pain on this haha, I am going to retract what I said before regarding querying the server. I understand the default timezone being the same on the server and the client is a nice to have, but not convinced it is worth the effort. Especially given that the timezone the user selects is now persisted on localstorage.

But yeah happy to also implement a server endpoint if thats what you want, I just wanted to point to an alternative solution (local storage) that is imo better because it is more simple.

[JPZ13]

Sorry to be a pain on this haha

You're not being a pain in the slightest @isubasinghe! Thanks for answering all my clarifying questions

I understand what you're getting at now. My apologies for the confusion on my end. Storing the timezone in local storage makes sense. Let's not worry about the API endpoint for now. If someone really wants it, we can always add it in later. Always better to ship small, incremental progress. I'll clone down the latest changes later today and give it a review

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

[isubasinghe]

not stale

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

[isubasinghe]

not stale