[amtool] update silence add and update flags#1298
Conversation
- Change --expires/-e to --duration/-d - Change --expires-on to --end - Add --start
The silences printed before were accurate, except they had the old ID. Now the new ID is returned.
|
@sinkingpoint I think you folks are the biggest users of |
|
@brian-brazil @simonpasquier any comments/objections? my plan is to do #1297 afterwards and have automated tests. So far I've only manually tested this code. |
|
Sounds fine by me. |
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
One question from me otherwise LGTM.
cli/silence_update.go
Outdated
| return nil, fmt.Errorf("silence duration must be greater than 0") | ||
| } | ||
| silence.EndsAt = time.Now().UTC().Add(time.Duration(duration)) | ||
| silence.EndsAt = silence.EndsAt.UTC().Add(time.Duration(d)) |
There was a problem hiding this comment.
It is now adding the passed duration to the existing EndsAt. I suppose this is intentional but it wasn't obvious from the change description.
There was a problem hiding this comment.
Hm, yeah, the original behavior was confusing to me. Now that I'm looking at this again, I'm thinking the duration should be added to StartsAt...
With amtool update <id> --start=<time> --duration=2h, I would expect this silence to be active for 2 hours after the start time.
With amtool update <id> --duration=2h I think it's less clear what the behavior should be. I would take this to mean "set the duration to start_time+2h, but I'm not sure if other folks agree?
There was a problem hiding this comment.
Yeah my gut feeling is that amtool update <id> --duration=<x> is ambiguous for users whichever time reference is chosen. Using StartsAt is probably the less confusing option but this is just my opinion ;-)
|
Sorry for the late reply. This is fine by us, but is there a justification for the flag renaming? I'd consider that a breaking change. (I'm aware that we're < 1.0 on this, but It'd be nice to have some reasoning behind it) |
|
It more closely maps to the field names used in the alertmanager frontend, and the flag name for the starting time |
When a user supplies a duration to update a silence, it is applied to silence.StartsAt after any potential changes to the silence's start time.
* Update silence add/update flags - Change --expires/-e to --duration/-d - Change --expires-on to --end - Add --start * update subcommand returns ID of new silence The silences printed before were accurate, except they had the old ID. Now the new ID is returned. * Duration is added to silence.StartsAt When a user supplies a duration to update a silence, it is applied to silence.StartsAt after any potential changes to the silence's start time.
4 participants
silence updatehas no default duration valuesilence updatenow returns the ID of the newly created silence, instead of printing the ID for the old silence.