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.

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

Return no error when deleting expired silence #2817

Merged
merged 14 commits into from
Feb 22, 2022
Merged

Return no error when deleting expired silence #2817

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d389091
Changed Silences.expire(id) to not return error for already expired s…
soonping-amzn Jan 12, 2022
342fef5
Added comment explaining idempotency change for Silences.expire()
soonping-amzn Jan 17, 2022
3ddd3e6
Trigger build
soonping-amzn Jan 18, 2022
1e7879f
Trigger build
soonping-amzn Jan 18, 2022
2a7ef46
Fixed typo in comment
soonping-amzn Jan 18, 2022
53a300e
Trigger build
soonping-amzn Jan 18, 2022
cc0283c
Trigger build
soonping-amzn Jan 18, 2022
e3f9f46
Fixed another typo in comment
soonping-amzn Jan 19, 2022
8e605a4
Promoted comment to function-level
soonping-amzn Jan 20, 2022
4b776b9
Added API v2 test for DeleteSilence, PostSilence
soonping-amzn Jan 26, 2022
5d0d557
Fixed lint errors
soonping-amzn Jan 26, 2022
619b390
Trigger build
soonping-amzn Jan 26, 2022
451452d
Trigger build
soonping-amzn Jan 26, 2022
4c36579
Trigger build
soonping-amzn Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion silence/silence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,8 @@ func (s *Silences) expire(id string) error {

switch getState(sil, now) {
case types.SilenceStateExpired:
return errors.Errorf("silence %s already expired", id)
Copy link
Member

@gotjosh gotjosh Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for coming in late - I feel that this kind of comment about the semantics of the function is best put as part of the function description.

How do you feel about changing the top-level comment from:

// Expire the silence with the given ID immediately.

to

// Expire the silence with the given ID immediately. It is idempotent, nil is returned if the silence already expired before it is GC'd.
// If the silence is not found an error is returned.

and removing the comment on the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I'll make the change.

// Returning nil ensures idempotent behaviour, at least in the short term before the silence is gc'd
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Returning nil ensures idempotent behaviour, at least in the short term before the silence is gc'd
// Returning nil ensures idempotent behaviour, at least in the short term before the silence is gc'd.

return nil
case types.SilenceStateActive:
sil.EndsAt = now
case types.SilenceStatePending:
Expand Down
8 changes: 2 additions & 6 deletions silence/silence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,9 +835,7 @@ func TestSilenceExpire(t *testing.T) {
require.NoError(t, s.Expire("pending"))
require.NoError(t, s.Expire("active"))

err = s.Expire("expired")
require.Error(t, err)
require.Contains(t, err.Error(), "already expired")
require.NoError(t, s.Expire("expired"))

sil, err := s.QueryOne(QIDs("pending"))
require.NoError(t, err)
Expand Down Expand Up @@ -935,9 +933,7 @@ func TestSilenceExpireWithZeroRetention(t *testing.T) {
require.NoError(t, s.Expire("pending"))
require.NoError(t, s.Expire("active"))

err = s.Expire("expired")
require.Error(t, err)
require.Contains(t, err.Error(), "already expired")
require.NoError(t, s.Expire("expired"))

_, err = s.QueryOne(QIDs("pending"))
require.NoError(t, err)
Expand Down