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

feat(template): add date template helper functions #5997

Merged
merged 15 commits into from
May 16, 2024

Conversation

ManAnRuck
Copy link
Contributor

@ManAnRuck ManAnRuck commented May 5, 2024

What this PR does / why we need it:

See Discord Thread for the details.

Which issue(s) this PR fixes:

Fixes #5995

Special notes for your reviewer:
do someone maybe have an idea to ensure the dates are always in the same timezone for the tests?

@eysi09
Copy link
Collaborator

eysi09 commented May 6, 2024

Thanks so much for the PR @ManAnRuck! ❤️

I think it definitely makes sense to have a helper function for printing the current timestamp with options for different formats.

Adding functionality for manipulating dates feels a little out of scope though tbh and I can see this grow in until we've re-implemented all of date-fs as template functions.

What exactly is your use case? Wondering if there's other ways to achieve that.

@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented May 6, 2024

Thanks so much for the PR @ManAnRuck! ❤️

I think it definitely makes sense to have a helper function for printing the current timestamp with options for different formats.

Adding functionality for manipulating dates feels a little out of scope though tbh and I can see this grow in until we've re-implemented all of date-fs as template functions.

What exactly is your use case? Wondering if there's other ways to achieve that.

Hi @eysi09 Thank you for your response :)

That would of course be ideal if we could find a solution without having to introduce additional complexity into the code :)

Specifically, the use case is that an environment variable exists for scraper services and api services, which defines the date from which the scraper or the data should be retrieved.

It is of course possible to enable this in the code itself using another environment variable, but this would be logic that is not part of the business logic of the actual service and I would therefore like to avoid incorporating it into the service.

For example, at the time the service/job is started, all data that is newer than 7 days for dev should be pulled. for production, the environment variable is set based on the date of the last update.

@ManAnRuck
Copy link
Contributor Author

Another alternative could be to make it possible to add your own scripts, which would be very good in the sense that it could work like a plugin system, but I imagine the implementation to be a bit more complex.

For the user it could then look like this: there is a standard file path or/and this can be defined in garden.yaml files.
This path then points to a file which exports an object of the type TemplateHelperFunction.
The content of the file could then be spread into the existing helperFunctionSpecs variable.

The biggest hurdles from my point of view would be the use of third party libraries for own functions (e.g. date-fns-tz) and the dynamic loading of the object without having to rebuild garden. I don't have an overview of this at the moment.

@eysi09
Copy link
Collaborator

eysi09 commented May 7, 2024

I discussed this a bit better with the team.

We agree that adding all these helpers functions is a lot and it kind of pollutes the function namespace because we don't have the functionality to tie to some context (e.g. datetime.parseDate). So in fact the majority of template functions would be simple date helpers.

Also note that we already have the datetime context.

Would it work for your use case if instead we just:

  • Keep the formatDate function
  • Add a catch all modifyDate function that can basically modify any date string

That modifyDate function could have the following signature:

modifyDate(timestamp: string, unit: "ms" | "s" | "d" | "m" | "y", value: number, format?: string)

The format character would be optional and only needed if the timestamp is not and ISO timestamp. The timestamp could then be passed through the date-fns parse function with the format if required.

It could be called like this in the YAML config:

yesterday: ${modifyDate(datetime.now, "d", -1)}

The formatDate function could then consume the output of modifyDate if needed.

@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented May 7, 2024

Thank you for the discussion and suggestions regarding the date handling functions. ❤️

I've taken the liberty to flesh out the modifyDate function as per the discussions, ensuring it accommodates both "add" and "set" modes. This allows for greater flexibility, whether adjusting or explicitly setting date units. To avoid ambiguity, especially in units that could be easily conflated like months ('m') and minutes ('minutes'), I opted to use fully spelled-out units. This should help prevent any potential confusion or errors in unit interpretation.

Additionally, I added a mode parameter that specifies whether to add to or set` the date unit, enhancing the function's adaptability for different use cases.

Regarding testing, I'm encountering an issue related to timezone handling. Currently, the function operates as expected only when the environment variable TZ='UTC' is set. Without this setting, local tests do not behave as intended. This should be fixed before merging 🧐 issue was date: joi.date()

yesterday: ${modifyDate(datetime.now, "days", -1)}
first-day: ${modifyDate(datetime.now, "days", 1, "set")}

I would be happy if you could rate these adjustments again and share your thoughts :)

(the code is already customized)

@ManAnRuck ManAnRuck marked this pull request as ready for review May 7, 2024 20:33
@ManAnRuck ManAnRuck changed the title [WIP] feat: add date template helper functions feat: add date template helper functions May 8, 2024
@vvagaytsev vvagaytsev changed the title feat: add date template helper functions feat(template): add date template helper functions May 14, 2024
@vvagaytsev
Copy link
Collaborator

@eysi09 @stefreak I've made some changes and added more test cases. Please review, see individual commits for details.

@vvagaytsev vvagaytsev marked this pull request as draft May 15, 2024 09:21
{ input: ["2021-01-01T00:00:00.234Z", 345, "milliseconds"], output: "2021-01-01T00:00:00.345Z" },
{ input: ["2021-01-01T00:00:05Z", 30, "seconds"], output: "2021-01-01T00:00:30.000Z" },
{ input: ["2021-01-01T00:01:00Z", 15, "minutes"], output: "2021-01-01T00:15:00.000Z" },
{ input: ["2021-01-01T12:00:00Z", 11, "hours"], output: "2021-01-01T11:00:00.000Z", skipTest: true },
Copy link
Member

Choose a reason for hiding this comment

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

These tests likely fail becaue we instantiate a date with local timezone from the UTC string, and then we modify the local time zone and export as UTC again.

In the first iteration of this, my proposal would be to rename shiftDate, modifyDate and formatDate to shiftDateUTC, modifyDateUTC and formatDateUTC and to only operate UTC dates; if someone needs timezone support later we can consider that by adding formatDateLocal or similar functions.

Copy link
Member

Choose a reason for hiding this comment

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

Do we not also need nowUTC() or something like that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests likely fail becaue we instantiate a date with local timezone from the UTC string, and then we modify the local time zone and export as UTC again.

In the first iteration of this, my proposal would be to rename shiftDate, modifyDate and formatDate to shiftDateUTC, modifyDateUTC and formatDateUTC and to only operate UTC dates; if someone needs timezone support later we can consider that by adding formatDateLocal or similar functions.

Did that for date modifiers in 890adc0. The formatDate will become formatDateUtc in the next commit :)

Do we not also need nowUTC() or something like that?

We can do that, it might be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the formatter was updated in 96b2300.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we need nowUtc() now. @ManAnRuck wdyt about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if it helps we can leave it out for now and from the user's point of view a workaround with a manual adjustment according to the time zone may have to be made first. Then we can first collect what problems may arise and improve this in a further integration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thank you! Let's collect some feedback and do the necessary improvement in the further PR(s).
We're merging this PR today :)

Manuel Ruck and others added 8 commits May 15, 2024 13:33
Signed-off-by: Manuel Ruck <git@manuelruck.de>
The hierarchical switch statements are hard to maintain.

The `mode` argument served as a routing param
to pick up necessary function behaviour.

It's better to get rid of it and to maintain 2 independent functions.
Each function can have its own limitations and time unit types.
TODO:
 * consider binding `ShiftDateTimeUnit` to `Duration` from `date-fns`
 * add more tests
Milliseconds are not supported by the underlying `add` function.
@vvagaytsev vvagaytsev requested a review from stefreak May 15, 2024 11:44
@vvagaytsev vvagaytsev marked this pull request as ready for review May 15, 2024 11:44
@vvagaytsev
Copy link
Collaborator

@stefreak @ManAnRuck please review the current version of the PR. Comments, feedback and improvement ideas are highly appreciated :)

cc @eysi09

@vvagaytsev vvagaytsev marked this pull request as draft May 15, 2024 14:11
@vvagaytsev vvagaytsev marked this pull request as ready for review May 15, 2024 14:30
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ManAnRuck, I think this can be helpful for some folks 👍
Thanks for revisiting @vvagaytsev

@vvagaytsev vvagaytsev added this pull request to the merge queue May 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 16, 2024
@vvagaytsev vvagaytsev added this pull request to the merge queue May 16, 2024
Merged via the queue into garden-io:main with commit 39d2396 May 16, 2024
17 checks passed
@ManAnRuck ManAnRuck deleted the date-functions branch May 18, 2024 15:01
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.

[FEATURE]: Enhanced Date Manipulation in Templating Files for Simplified Environment Configuration
4 participants