Skip to content

filter: add new time_delta filter #9831

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Mar 3, 2025

SUMMARY
  • Calculate datetime string based upon time delta
    provided by user

Signed-off-by: Abhijeet Kasurde [email protected]

ISSUE TYPE
  • Feature Pull Request

@ansibullbot
Copy link
Collaborator

cc @resmo
click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request filter filter plugin integration tests/integration needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) tests tests labels Mar 3, 2025
@Akasurde Akasurde force-pushed the arrow branch 2 times, most recently from 9700872 to 7b5c05f Compare March 3, 2025 20:55
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Mar 3, 2025
@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Mar 3, 2025
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Mar 3, 2025
@resmo
Copy link
Member

resmo commented Mar 3, 2025

nice

* Calculate datetime string based upon time delta
  provided by user

Signed-off-by: Abhijeet Kasurde <[email protected]>
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 3, 2025
@russoz
Copy link
Collaborator

russoz commented Mar 5, 2025

Whilst I appreciate the idea very much, the name time_delta made me think two dates would be given and the difference/delta between them would be calculated. Maybe something like time_add_delta ? Update: or even time_add?

Additionally, I think forcing this to use string representation is limiting. We can have datetime objects and time delta objects, as shown in:
https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_filters.html#handling-dates-and-times)

Therefore, I reckon it would be interesting to:

  • allow datetime objects to be passed (through the pipe operator), forsaking the need to specify a format
  • allow time delta objects to be passed as parameters instead of forcing users to specify days, weeks or whatever.

I mean those in addition to the current code, which is clearly quite useful.

WDYT?

Copy link
Collaborator

@felixfontein felixfontein 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 your contribution! I agree with @russoz that the name should be adjusted; I was thinking as well that this filter allows to compute the difference between two timestamps, and not to modify a timestamp.

raise AnsibleFilterError('time_delta accepts datetime in string format')
date_format = "%Y-%m-%d %H:%M:%S"
if 'date_format' in kwargs:
date_format = kwargs.get('date_format')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be validated to be a string.

'minutes': kwargs.get('minutes', 0),
'hours': kwargs.get('hours', 0),
'weeks': kwargs.get('weeks', 0),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to validate the above to be integers if supplied as options. Also you should check whether kwargs has more arguments than the ones we use (and fail if there are more).

f'Failed to parse provided string into datetime format "{date_format}" provided.'
)

return str(source_date + timedelta(**delta))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't date_format be used to format the timestamp?

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Mar 17, 2025
@VannTen
Copy link
Contributor

VannTen commented Apr 11, 2025

A few thought on this:
Ansible has the to_datetime builtin filter which can produces datetime object, which you can use to obtain a timedelta object:

---
- gather_facts: no
  hosts: localhost
  connection: local
  vars:
    date1: '00:00'
    date2: '01:00'
    fmt: '%H:%M'
    timedelta: "{{ (date2 | to_datetime(fmt)) - (date1 | to_datetime(fmt)) }}" # this is coerced to string, you can't "store" your timedelta
  tasks:
    - debug:
        var: now() + ((date2 | to_datetime(fmt)) - (date1 | to_datetime(fmt)))

IMO, instead of a filter applying a computed timedelta on a datetime, it would be better to have a way to directly construct a timedelta. This allows more things, like comparing timedeltas, doing arithemic on it etc

(example usecase: checking if remaining validity time of a certificate is greater than 20% of it's total duration, that sort of thing)

I don't think there is a way to add jinja functions as extension (is there ?), but maybe a filter passing a dict to the timedelta constructor would do it.
That would change the example above to :

---
- gather_facts: no
  hosts: localhost
  connection: local
  tasks:
    - debug:
        var: "now() + ({'hours': 1} | community.general.timedelta)" # NOT IMPLEMENTED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request filter filter plugin has_issue integration tests/integration plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants