Add support for alerts, notifiers and add additional ingest-token support. #12
Conversation
…port. API: Return tag fields when creating a parser. API: Fix types for repo retention settings. CLI: Fix repo list ordering. CLI: Fix repo retention size output. Tidy up based on different linters. Some day we should enable these things on CI :-) - Receiver reference are typically named based on the type. - Remove unused functions and structs. - Acronyms are typically upper case. - All returned errors should be checked, though we may have places we don't care as much about it.
SaaldjorMike
force-pushed
the
mike/add_alerts_notifiers_etc
branch
from
6790786 to
6962c3b
Compare
api/alerts.go
Outdated
| return a.Update(viewName, alert) | ||
| } | ||
|
|
||
| url := fmt.Sprintf("api/v1/repositories/%s/alerts/", viewName) |
There was a problem hiding this comment.
There is some duplication between this Add() and the Update(). Perhaps there should be a private method that handles the code from this line down which is then called by these functions.
| return &resNotifier, nil | ||
| } | ||
|
|
||
| func (n *Notifiers) Add(viewName string, notifier *Notifier, force bool) (*Notifier, error) { |
There was a problem hiding this comment.
Same as the above with the Add() and Update() methods (duplication). As well as the duplication around the unmarshalling in the other public methods here.
There was a problem hiding this comment.
After pulling out the marshal/unmarshal code, I'm not sure it would help that much to move this out as well. I'm thinking of just leaving this as-is unless you think I should still do it.
| cmd.Println(fmt.Errorf("Failed to serialize the alert: %s", yamlErr)) | ||
| os.Exit(1) | ||
| } | ||
| outFilePath := outputName + ".yaml" |
There was a problem hiding this comment.
I think this should be outputFilename rather than outputName + '.yaml'. It seems a little awkward to append the file suffix like this.
There was a problem hiding this comment.
This actually just comes from how the current export function is done for parsers. See the code.
Just to make sure I understood you: you propose renaming outFilePath to outputFilename?
There was a problem hiding this comment.
Ah, I suppose we should keep it the same to be consistent.
| cmd.Println(fmt.Errorf("Failed to serialize the notifier: %s", yamlErr)) | ||
| os.Exit(1) | ||
| } | ||
| outFilePath := outputName + ".yaml" |
|
Merging this in now. Should there be any additional changes, we can fix those before cutting the next release. |
Changes:
apipackage:I'm still a bit unsure what to do about a few things, and would love to get some input on these things:
apipackage: view vs viewName vs repo vs repoName vs repositoryName vs searchDomain. For now I've used viewName for alerts and notifiers, even though it may be a tad confusing that it can also be used on a repository.