Conversation
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
Thanks for submitting!
One additional question: What was your motivation for moving all the cli files into their own cli/cmd directory? To me, cmd is a bit confusing as I always associate it with a top level cmd/<binary-name/main.go structure, where it contains something that will result in a binary for consumers.
It might make more sense to move those files back to cli/, and then move cli/client.go to its own client/ dir. I'm guessing that was the separation you wanted, between "cli stuff" and "the client that interacts with the api".
Thanks for all the work!
cli/client.go
Outdated
| type AlertAPI interface { | ||
| // List returns all the active alerts. | ||
| List(ctx context.Context) ([]*ExtendedAlert, error) | ||
| List(ctx context.Context, filter string, silenced bool, inhibited bool) ([]*ExtendedAlert, error) |
There was a problem hiding this comment.
List(ctx context.Context, filter string, silenced, inhibited bool) ([]*ExtendedAlert, error)
cli/client.go
Outdated
| } | ||
|
|
||
| func (h *httpAlertAPI) List(ctx context.Context) ([]*ExtendedAlert, error) { | ||
| func (h *httpAlertAPI) List(ctx context.Context, filter string, silenced bool, inhibited bool) ([]*ExtendedAlert, error) { |
There was a problem hiding this comment.
same as above, you only need to declare bool the one time
cli/cmd/silence_import.go
Outdated
| } | ||
|
|
||
| func addSilence(silence *types.Silence) (string, error) { | ||
| client, err := api.NewClient(api.Config{Address: (*alertmanagerUrl).String()}) |
There was a problem hiding this comment.
I think most of this function can disappear. It makes sense to me to create the client + silence client once, and then pass the silence client into each addSilenceWorker function. A bulk import could potentially have thousands of silences, which in its current form would create thousands of clients when only one is needed.
With this, the line silenceID, err := addSilence(s) in addSilenceWorker could just be silenceID, err := silenceAPI.Set(context.Background(), *silence)
👍 I'll update accordingly. Thanks for the other tips. |
And move API client code to its own package.
* cli: move commands to cli/cmd * cli: use StatusAPI interface for config command * cli: use SilenceAPI interface for silence commands * cli: use AlertAPI for alert command * cli: move back commands to cli package And move API client code to its own package. * cli: remove unused structs
2 participants
This PR modifies the cli commands to use the newly added API client.
cc @stuartnelson3