ui: Move /status & /silences to API v2#1613
Conversation
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
All of these maybe's (like for silence ID) represent (to me) not trusting the API, or having a strong contract of what will or won't be there. If we can guarantee these values will be present when retrieved from the API, a model with types that represent this would make the code much cleaner. Given that we have strongly typed decoding, we would also catch such errors early.
Can we be confident in this contract and represent it in the code?
ui/app/src/Silences/Api.elm
Outdated
| url = | ||
| String.join "/" [ apiUrl, "silence", silence.id ] | ||
| -- TODO: Maybe.withDefault is not perfect. Silences should always | ||
| -- have an id, what should we do? |
There was a problem hiding this comment.
The API should fail and provide a good error message, which we can render? I don't think we can get to this branch without having a valid silence with an ID, so I don't think this can happen, as you stated
|
|
||
|
|
||
|
|
||
| {- |
There was a problem hiding this comment.
This can be removed?
|
Thanks for doing all this. We haven't touched the elm code in a while, but it's really nice coming back to this language and feeling confident in the changes we make thanks to the type system :) |
This patch makes the Alertmanager UI (/status & /silences) use the api/v2 endpoint. In addition it adds logic to generate the elm side data model based on the OpenAPI specification. Signed-off-by: Max Leonard Inden <IndenML@gmail.com>
With the previous patch /status and /silences were requested from api v2. Requesting alerts from api v1 is done in a separate commit to be able to revert it once alerts also come from api v2. Signed-off-by: Max Leonard Inden <IndenML@gmail.com>
mxinden
force-pushed
the
api-v2-silences-status
branch
4 times, most recently
from
82fef5f to
111ce11
Compare
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
OpenAPI has the allOf keyword to achieve composition, similar to Golangs composition.
Instead of having one type Silence for both GET and POST, we now have one base type Silence. In addition we have the GettableSilence and PostableSilence which are composed off the base type Silence and all additional fields.
type alias PostableSilence =
{ matchers : Matchers
, startsAt : DateTime
, endsAt : DateTime
, createdBy : String
, comment : String
-- Needs to be of type `Maybe` to update existing silences.
, id : Maybe String
}
type alias GettableSilence =
{ matchers : Matchers
, startsAt : DateTime
, endsAt : DateTime
, createdBy : String
, comment : String
, id : String
, status : SilenceStatus
, updatedAt : DateTime
}This leaves us with a single Maybe. Thanks @w0rm & @stuartnelson3, I like this
much better. Any further thoughts?
|
|
||
|
|
||
| type alias Alert = | ||
| { startsAt : Maybe DateTime |
There was a problem hiding this comment.
These will change once I move /alerts to the new API as well.
|
Looks good to me! |
w0rm
left a comment
w0rm
left a comment
There was a problem hiding this comment.
Looks great, thanks for iterating on this!
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
👍 once the build is green
Instead of having one general silence, differentiate between postable and gettable silence, hence making more fields required. Signed-off-by: Max Leonard Inden <IndenML@gmail.com>
mxinden
force-pushed
the
api-v2-silences-status
branch
from
111ce11 to
b4b8b75
Compare
|
Thanks @w0rm and @stuartnelson3. I will follow up with moving the |
3 participants
This patch makes the Alertmanager UI (/status & /silences) use the
api/v2 endpoint. In addition it adds logic to generate the elm side data
model based on the OpenAPI specification.
In addition requesting alerts from api v1 is done in a separate commit to be
able to revert it once alerts also come from api v2.