feat: timeinterval #285
feat: timeinterval #285
Conversation
WalkthroughAdds a new time-interval engine package for parsing/evaluating complex time intervals and extends the properties package with accessors to read JSON-encoded time-interval values (default key Changes
Sequence Diagram(s)mermaid Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @properties/properties.go:
- Around line 302-323: The documentation example in the TimeIntervals function
is wrong: it shows a single JSON object but the code unmarshals into
[]timeinterval.TimeInterval; update the comment example to show a JSON array
(e.g. [{"weekdays":["monday:friday"],"times":[...]}]) OR change TimeIntervals to
accept both forms by attempting json.Unmarshal into []timeinterval.TimeInterval
and, on UnmarshalTypeError or if the result is empty, try unmarshalling into a
single timeinterval.TimeInterval and append it to the slice before continuing;
adjust error messages accordingly and keep references to the function
TimeIntervals and the propKey "time_interval."+key.
In @timeinterval/timeinterval.go:
- Around line 445-447: Fix the typo in the comment for the MarshalText method:
change "econding.TextMarshaler" to "encoding.TextMarshaler" in the comment above
the Location.MarshalText function so the doc correctly references the
encoding.TextMarshaler interface.
- Around line 257-259: The error message for the check in the method where you
compare r.Begin and r.End is inverted: when r.Begin > r.End the message should
state the start/begin cannot be after the end, not "before"; update the error
returned in that conditional (the one using r.Begin > r.End) to a correct
message such as "start day cannot be after end day" or "begin cannot be after
end" so it accurately reflects the failing condition.
- Around line 398-401: Fix the typo in the comment above
WeekdayRange.MarshalText: change "econding.TextMarshaler" to
"encoding.TextMarshaler" so the doc correctly references the
encoding.TextMarshaler interface for the MarshalText method on WeekdayRange.
- Around line 609-611: The boundary check currently allows minute value 60
because it uses timeStampMinutes > 60; update the condition in the validation
`if timeStampHours < 0 || timeStampHours > 24 || timeStampMinutes < 0 ||
timeStampMinutes > 60` to use `timeStampMinutes >= 60` (i.e., `... ||
timeStampMinutes >= 60`) so values 60 are rejected; keep the same error return
(`return 0, fmt.Errorf("timestamp %s out of range", in)`) and ensure you update
the condition referencing the variables timeStampHours, timeStampMinutes and the
input `in`.
🧹 Nitpick comments (1)
properties/properties.go (1)
252-254: Returningerrwhen it's guaranteed to be nil is confusing.At this point,
erris alwaysnil(the error case was handled above). Consider returningnil, nilexplicitly for clarity.🔎 Proposed fix
if len(hours) == 0 { - return nil, err + return nil, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
properties/properties.gotimeinterval/timeinterval.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Run linting usingmake lintto enforce golangci-lint with project-specific rules that disable certain checks for HTTP-related naming
Use the logger package for all logging needs; avoid fmt.Print statements
Use explicit error returns rather than panics
Logger and Properties packages use global instances initialized viaInit()functions; always check if initialization is needed before using these packages
When adding new functionality, prefer using existing utilities from within the project (e.g., usecollectionspackage for slice operations)
Files:
properties/properties.gotimeinterval/timeinterval.go
🧠 Learnings (1)
📚 Learning: 2026-01-02T13:47:24.505Z
Learnt from: CR
Repo: flanksource/commons PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T13:47:24.505Z
Learning: Applies to **/*.go : Logger and Properties packages use global instances initialized via `Init()` functions; always check if initialization is needed before using these packages
Applied to files:
properties/properties.go
🧬 Code graph analysis (1)
properties/properties.go (1)
timeinterval/timeinterval.go (2)
TimeIntervals(61-61)TimeInterval(75-82)
🔇 Additional comments (5)
timeinterval/timeinterval.go (3)
37-53: LGTM!The Mutes method correctly iterates through named intervals, checks time containment with UTC conversion, and returns matched names. The UTC conversion here works well with the Location handling in ContainsTime.
61-125: LGTM!Well-structured type definitions with clean use of embedding for the various range types. The TimeInterval struct covers all necessary constraint dimensions.
541-543: Clamp minimum bound appears incorrect for day-of-month calculations.After converting negative indices to positive on lines 527-536,
beginandendare positive values representing actual day numbers. Using-1*daysInMonthas the minimum bound doesn't align with the positive day number domain. The minimum should be1to represent valid days.Although input validation (lines 286-291) likely prevents this from causing issues in practice, the clamping logic could mask bugs or cause unexpected behavior in edge cases.
🔎 Proposed fix
- begin = clamp(begin, -1*daysInMonth, daysInMonth) - end = clamp(end, -1*daysInMonth, daysInMonth) + begin = clamp(begin, 1, daysInMonth) + end = clamp(end, 1, daysInMonth)properties/properties.go (2)
69-73: LGTM!The new import and constant follow the existing patterns in the codebase. The constant
businessHoursKeyprovides a single source of truth for the default key.
259-261: LGTM!The global wrapper function follows the established pattern used by other accessor functions in this package.
adityathebe
force-pushed
the
feat/timeinterval
branch
from
457dbc8 to
554b559
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
timeinterval/timeinterval.go (4)
257-259: Error message is inverted.The condition checks
r.Begin > r.End, but the error says "start day cannot be before end day". When Begin > End, the start is after the end, not before.🔎 Proposed fix
if r.Begin > r.End { - return errors.New("start day cannot be before end day") + return errors.New("start day cannot be after end day") }
398-401: Typo in comment: "econding" → "encoding".🔎 Proposed fix
-// MarshalText implements the econding.TextMarshaler interface for WeekdayRange. +// MarshalText implements the encoding.TextMarshaler interface for WeekdayRange.
445-447: Typo in comment: "econding" → "encoding".🔎 Proposed fix
-// MarshalText implements the econding.TextMarshaler interface for Location. +// MarshalText implements the encoding.TextMarshaler interface for Location.
609-611: Boundary check should use>= 60for minutes.The validation
timeStampMinutes > 60incorrectly allows the value 60. Minutes should be in the range 0-59, so the check should be>= 60.🔎 Proposed fix
- if timeStampHours < 0 || timeStampHours > 24 || timeStampMinutes < 0 || timeStampMinutes > 60 { + if timeStampHours < 0 || timeStampHours > 24 || timeStampMinutes < 0 || timeStampMinutes >= 60 {
🧹 Nitpick comments (1)
timeinterval/timeinterval.go (1)
617-644: Variable name "End" should be lowercase.On line 629, the variable is named
End(capital E), which is inconsistent with Go naming conventions for local variables. It should beend.🔎 Proposed fix
- End, err := r.memberFromString(components[1]) + end, err := r.memberFromString(components[1]) if err != nil { return err } r.setBegin(start) - r.setEnd(End) + r.setEnd(end)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
properties/properties.gotimeinterval/timeinterval.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Run linting usingmake lintto enforce golangci-lint with project-specific rules that disable certain checks for HTTP-related naming
Use the logger package for all logging needs; avoid fmt.Print statements
Use explicit error returns rather than panics
Logger and Properties packages use global instances initialized viaInit()functions; always check if initialization is needed before using these packages
When adding new functionality, prefer using existing utilities from within the project (e.g., usecollectionspackage for slice operations)
Files:
properties/properties.gotimeinterval/timeinterval.go
🧠 Learnings (1)
📚 Learning: 2026-01-02T13:47:24.505Z
Learnt from: CR
Repo: flanksource/commons PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T13:47:24.505Z
Learning: Applies to **/*.go : Logger and Properties packages use global instances initialized via `Init()` functions; always check if initialization is needed before using these packages
Applied to files:
properties/properties.go
🧬 Code graph analysis (1)
properties/properties.go (1)
timeinterval/timeinterval.go (2)
TimeIntervals(61-61)TimeInterval(75-82)
🔇 Additional comments (2)
timeinterval/timeinterval.go (1)
220-246: LGTM: Good Windows platform handling.The special handling for Windows with the ZONEINFO environment variable is helpful, as Windows doesn't ship with a bundled timezone database by default.
properties/properties.go (1)
302-328: LGTM: Time intervals integration is well-implemented.The method correctly:
- Defaults to "business_hours" key when no keys provided
- Aggregates intervals from multiple property keys
- Uses appropriate error wrapping with context
- Skips empty values gracefully
The documentation example now correctly shows array format matching the implementation.
|
related: |
|
🎉 This PR is included in version 1.44.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Example:
time_interval.business_hours=[{"weekdays":["monday:friday"],"times":[{"start_time":"09:00","end_time":"17:00"}],"location":"Asia/Kathmandu"}]used this time format: https://prometheus.io/docs/alerting/latest/configuration/#time_interval
Example:
Accept from properties for now and then we can move to CRD or tenant wide UI settings
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.