node icon indicating copy to clipboard operation
node copied to clipboard

Add timezone update workflow

Open 98lenvi opened this issue 1 year ago • 4 comments

Currently, this script checks the timezone version of the runner (instead it should be checking from elsewhere, I will check on how to do that). I haven't tested this by running an action yet.

Creating this PR to get early feedback

Edit 1 Just discovered node/tools/icu-data, will get the data from there instead & update the PR

Edit 2 tested this action in my fork. Looks okay to me

Resolves 43134

98lenvi avatar Jul 25 '22 17:07 98lenvi

Review requested:

  • [ ] @nodejs/actions

nodejs-github-bot avatar Jul 25 '22 17:07 nodejs-github-bot

What should be the frequency of the workflow? It currently is set to every Sunday. I think we should set it to run every day.

98lenvi avatar Jul 27 '22 02:07 98lenvi

What should be the frequency of the workflow? It currently is set to every Sunday. I think we should set it to run every day.

We only release new versions of Node.js every two weeks or so, I don't think we'd get much by increasing the frequency (and the downsides would be more noise, and less overall available GHA runners).

aduh95 avatar Jul 27 '22 06:07 aduh95

Should we add a test? We can check the value of process.versions.tz in the test, and whenever the workflow updates the ICU file, we will update the test as well. The test will serve two purposes

  • test if the tz version is really updated
  • A place in the codebase that clearly tells the tz version. Right now only after we build Nodejs & run it, we get to know the tz version.

98lenvi avatar Jul 27 '22 13:07 98lenvi

Hey! I've read the original discussion in #43134 , and I was wondering if you might need any help?

I am also new to open source projects and I would very much like to learn how to contribute!

NourABSH avatar Aug 16 '22 09:08 NourABSH

@NourABSH I’m waiting for additional feedback on this diff, but if you can find a better way to tackle this, then please feel free to create a new diff, I’ll close this one after you open yours.

98lenvi avatar Aug 16 '22 13:08 98lenvi

Hi @srl295, you mentioned me in #43134 how can i help in this issue

yashdev9274 avatar Aug 18 '22 11:08 yashdev9274

Hi @srl295, you mentioned me in #43134 how can i help in this issue

perhaps by reviewing or even testing the code in this PR?

srl295 avatar Aug 18 '22 16:08 srl295

is this still open

yashdev9274 avatar Aug 20 '22 18:08 yashdev9274

is this project assigned to anyone

yashdev9274 avatar Aug 20 '22 18:08 yashdev9274

@yumaoka do you have any comments on this approach?

srl295 avatar Aug 30 '22 17:08 srl295

FYI @nodejs/i18n-api

srl295 avatar Aug 30 '22 17:08 srl295

What should be the frequency of the workflow? It currently is set to every Sunday. I think we should set it to run every day.

time zone releases aren't every day. If it's weekly it should cache the icudata repo

srl295 avatar Aug 30 '22 17:08 srl295

Landed in https://github.com/nodejs/node/commit/85b46e1cab0f91eb87ff30f14bee0b456a63c83b, thanks for the contribution 🎉

aduh95 avatar Sep 15 '22 22:09 aduh95

Please ping me if you have questions on it !

srl295 avatar Sep 16 '22 02:09 srl295