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

TimeRange: Parse duration with d/M/y unit #1212

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AgnesToulet
Copy link

What this PR does / why we need it:
This PR updates the parse function to use our own ParseDuration function instead of the Golang one. The Golang one doesn't handle unit like d, M or y that are valid units in Grafana frontend when setting a panel relative time range:
image

Which issue(s) this PR fixes:

https://github.com/grafana/support-escalations/issues/14375

Special notes for your reviewer:

  • I tested it for the escalation use case and it's working but I wonder if there are any other use cases where this could cause an unexpected behavior.
  • This would require a more complex fix if we want to handle daylight saving time/leap years/proper month duration... But I don't think it's worth it as this can already work by using the syntax now-1M. Maybe we should update the doc to recommend using the second syntax instead of the first one?

@AgnesToulet AgnesToulet requested review from a team as code owners January 31, 2025 15:56
@AgnesToulet AgnesToulet requested review from alyssabull and removed request for a team January 31, 2025 15:56
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@matyax matyax requested a review from svennergr January 31, 2025 16:00
@matyax
Copy link

matyax commented Jan 31, 2025

This seems to be a shared component with Loki. If you don't mind, I added @svennergr as reviewer to make sure this doesn't cause any unexpected regression.

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.

3 participants