Skip to content

Fix child routes not inheriting time interval configuration #3538

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.

Sign up for GitHub

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

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

Fix child routes not inheriting time interval configuration #3538

benridley wants to merge 2 commits into from

Conversation

@benridley
Copy link
Contributor

@benridley benridley commented Sep 29, 2023

Fixes #2833.

Fixes a bug in the creation of routes which would mistakenly override parent route config with empty time interval configuration if none were specified on the child.

@benridley
Fix child routes not inheriting time interval configuration
2269bbc 
Fixes a bug in the creation of routes which would mistakenly override parent route config with empty time interval configuration if none were specified on the child.

Signed-off-by: Ben Ridley <benridley29@gmail.com>
This was referenced Sep 29, 2023
Open
Open
@benridley
Add default time interval config to match test cases
148f4a8 
Signed-off-by: Ben Ridley <benridley29@gmail.com>
@benridley benridley force-pushed the bugfix_time_interval_inheritance branch from 5f93ef6 to 148f4a8 Compare September 29, 2023 00:26
@juliusv
Copy link
Member

juliusv commented Oct 14, 2023

Thanks! I haven't checked in detail, but @simonpasquier mentioned in #2833 (comment) that overriding time intervals in children could lead to issues because empty time intervals are also valid. Would setting an explicit empty list of intervals (vs. omitting the field entirely) in a child allow overriding the time intervals of a parent with this PR?

@benridley
Copy link
Contributor Author

benridley commented Oct 17, 2023

Thanks! I haven't checked in detail, but @simonpasquier mentioned in #2833 (comment) that overriding time intervals in children could lead to issues because empty time intervals are also valid. Would setting an explicit empty list of intervals (vs. omitting the field entirely) in a child allow overriding the time intervals of a parent with this PR?

Maybe I'm misunderstanding the implications of this, but I think this shouldn't have any impact. Previously, every new route would receive an explicit empty list of intervals (through the DefaultRouteOpts struct) regardless of whether the parent had a set of time intervals or not. Now, child route will receive an empty list, or the parent's list if its not empty.

So empty time intervals on the parent will result in the same behaviour before this change. The change will be if the parent has defined intervals which will now be inherited.

@juliusv juliusv requested a review from simonpasquier October 23, 2023 12:57
@costela
Copy link

costela commented Sep 16, 2024

Friendly ping: the review seems to have fallen through the cracks? 😇

aither64 added a commit to vpsfreecz/vpsfree-cz-configuration that referenced this pull request Dec 15, 2025
@aither64
alerter: set active_time_intervals on leaf routes
86dcb35 
Apparently, active_time_intervals setting is not inherited by child
routes, see:

  prometheus/alertmanager#2833
  prometheus/alertmanager#3538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simonpasquier simonpasquier Awaiting requested review from simonpasquier

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mute_time_intervals should be inherited to sub routes

3 participants

@benridley @juliusv @costela