Skip to content

Refactor: Move inTimeIntervals from notify to timeinterval#3556

Merged
gotjosh merged 4 commits intomainfrom
refactor-timeinterval-in-notify
Oct 13, 2023
Merged

Refactor: Move inTimeIntervals from notify to timeinterval#3556
gotjosh merged 4 commits intomainfrom
refactor-timeinterval-in-notify

Conversation

@gotjosh
Copy link
Member

@gotjosh gotjosh commented Oct 12, 2023

This should make implementing #3520 a bit cleaner given that we shouldn't need to introduce a direct dependency on marker in notify.

There's absolutely no change of functionality here and I've expanded coverage for similar logic in both places.

Signed-off-by: gotjosh <josue.abreu@gmail.com>
Signed-off-by: gotjosh <josue.abreu@gmail.com>
timeIntervals[ti.Name] = ti.TimeIntervals
}

intervener := timeinterval.NewIntervener(timeIntervals)
Copy link
Member Author

Choose a reason for hiding this comment

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

I started moving the loop above into timeinterval as well but quickly realised that we have an import problem - this is part of a larger problem and I'll address it in a separate PR.

I don't understand why the marshalling of config -> struct happens with structs that live in timeinterval when this is generally done as part of theconfig package.

Comment on lines +893 to +895
if len(muteTimeIntervalNames) == 0 {
return ctx, alerts, nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This guard clause was missing from this path.

Signed-off-by: gotjosh <josue.abreu@gmail.com>
Signed-off-by: gotjosh <josue.abreu@gmail.com>
return loc
}

func TestIntervener_Mutes(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests are technically a duplicate of what we already have in notify - I think they have some merit in being here as there might be more we want to test as part of this interface in the future (e.g. when we introduce the marker) and it gave me more confidence in the refactor.

@gotjosh gotjosh marked this pull request as ready for review October 12, 2023 15:03
@gotjosh gotjosh requested a review from simonpasquier October 12, 2023 15:06
Copy link
Collaborator

@grobinson-grafana grobinson-grafana left a comment

Choose a reason for hiding this comment

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

LGTM!

@gotjosh gotjosh merged commit acb5840 into main Oct 13, 2023
@gotjosh gotjosh deleted the refactor-timeinterval-in-notify branch October 13, 2023 13:15
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
…metheus#3556)

* Refactor: Move `inTimeIntervals` from `notify` to `timeinterval`

There's absolutely no change of functionality here and I've expanded coverage for similar logic in both places.
---------

Signed-off-by: gotjosh <josue.abreu@gmail.com>
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.

2 participants