Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Update Alertmanager models #44

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Update Alertmanager models #44

merged 2 commits into from
Apr 7, 2021

Conversation

gotjosh
Copy link
Contributor

@gotjosh gotjosh commented Apr 7, 2021

Update the Alertmanager models to add necessary ones for the /alerts and /alerts/groups endpoints.

@gotjosh
Copy link
Contributor Author

gotjosh commented Apr 7, 2021

I genuinely will never understand go mod. The difference in dependencies is produced by running go mod tidy and then go mod vendor.

I'm not sure if it's because I have a different set of dependencies or because we forgot to converge at some other point - any pointers?

@gotjosh gotjosh requested review from owen-d and papagian April 7, 2021 10:57
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

No idea about the go mod changes, but would not worry about it looking at it

@@ -119,10 +119,19 @@ type GettableSilences = amv2.GettableSilences
type GettableSilence = amv2.GettableSilence

// swagger:model
type GettableAlerts amv2.GettableAlerts
type GettableAlerts = amv2.GettableAlerts
Copy link
Member

Choose a reason for hiding this comment

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

This is the first time I am seeing type x = y syntax 😱 how does it work?

Copy link
Member

Choose a reason for hiding this comment

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

Is it equivalent to type x y?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find the original article I read on the subject which does a better job at explaining than I do, but it is different. The tl;dr is that type x y is a type definition, in the eyes of the compiler is a new type just like type X struct { y } whereas type x = y is type alias which means is just a different name for the same type (and thus would have the same memory footprint).

In practical terms, this allows us to use our types and the returned AM types interchangeably w/o having to do any type casting. We are not operating on these types so it makes sense.

Copy link
Collaborator

@papagian papagian left a comment

Choose a reason for hiding this comment

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

The modifications LGTM
I don't know about the go dependencies but I get the same result if run go mod tidy in master.
Can you please post also the update spec.json and post.json?

@gotjosh
Copy link
Contributor Author

gotjosh commented Apr 7, 2021

@papagian on a closer inspection, I found a couple more inconsistencies with the upstream API could I please get another look?

@@ -106,7 +106,7 @@ type GetDeleteSilenceParams struct {
// swagger:parameters RouteGetSilences
type GetSilencesParams struct {
// in:query
Filter []string
Filter []string `json:"filter"`
Copy link
Collaborator

@papagian papagian Apr 7, 2021

Choose a reason for hiding this comment

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

Does this mean that the query parameter will become filter instead of Filter in this request, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I have already fixed this with https://github.com/grafana/grafana/pull/32604/files

// in: query
// required: false
Receivers []string `json:"receivers"`
Receivers string `json:"receiver"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference, these changes are required for conformance with this API route

@gotjosh gotjosh merged commit 64bd267 into master Apr 7, 2021
@gotjosh gotjosh deleted the alertmanager-models-update branch April 7, 2021 15:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants