ui: Move UI to API v2 #1560

mxinden · 2018-09-26T10:28:20Z

This patch makes the Alertmanager UI use the api/v2 endpoint. In
addition it adds logic to generate the elm side data model based on the
OpenAPI specification.

//CC @stuartnelson3 as discussed via e-mail.

stuartnelson3 · 2018-09-26T10:30:08Z

ui/app/elm.json

@@ -15,7 +15,8 @@
            "elm/regex": "1.0.0",
            "elm/time": "1.0.0",
            "elm/url": "1.0.0",
-            "rtfeldman/elm-iso8601-date-strings": "1.1.2"
+            "rtfeldman/elm-iso8601-date-strings": "1.1.2",
+	    "NoRedInk/elm-json-decode-pipeline":    "1.0.0"


the alignment on this looks funny, this was added automatically?

ui/app/src/Data/Alert.elm

stuartnelson3 · 2018-09-26T10:32:35Z

ui/app/src/Data/InlineResponse200.elm

+-}
+
+
+module Data.InlineResponse200 exposing (InlineResponse200, inlineResponse200Decoder, inlineResponse200Encoder)


funny, looks like a github syntax highlighting bug?

ui/app/src/Request/Alert.elm

stuartnelson3

Only question I have right now is about how to use the generated methods for making HTTP requests to endpoints, since it looks like localhost is hardcoded in as the baseUrl.

Awesome to see this, thanks for pushing openapi through!

mxinden · 2018-09-26T10:46:11Z

Only question I have right now is about how to use the generated methods for making HTTP requests to endpoints, since it looks like localhost is hardcoded in as the baseUrl.

I am still trying to figure this out. So far I have only used the generated data models and decoders due to that. But using the generated request logic would also be nice.

mxinden · 2018-09-28T20:47:22Z

Relates to: OpenAPITools/openapi-generator#1140

mxinden · 2018-10-18T11:52:51Z

@stuartnelson3 @w0rm I refactored all of the UI code to use the new API v2 with the generated model logic. This is still in work-in-progress but I would love your feedback. Especially everything marked with TODO. As my elm skills are a bit rusty feedback is very welcome.

mxinden · 2018-10-18T12:04:50Z

From my side this is the last thing missing before we can cut the first alpha or beta in #1553.

w0rm · 2018-10-18T12:36:04Z

ui/app/src/Views/AlertList/AlertView.elm


        Nothing ->
+            -- TODO: What to return here? This case should never happen.


If this should never happen, then why is status a maybe type?

The definition of Silence is generated based on the new OpenAPI specification of the API v2. I would prefer not to have an additional internal type which converts to the generated one.

Why does the specification not enforce the status property: E.g. when creating a silence that status property is not yet known. Thereby the API does not enforce the property.

Is that enough context? What are your thoughts?

I see, shouldn't a silence to be created and a silence that we retrieve be two different types?

w0rm · 2018-10-18T12:40:39Z

ui/app/src/Views/SilenceList/SilenceView.elm

-            in
+        editUrl =
+            -- TODO: silence.id should always be set. Can this be done nicer?
+            String.join "/" [ "#/silences", Maybe.withDefault "" silence.id, "edit" ]


If the id is undefined, we could maybe generate a link to create a new silence?

The id would never be undefined, as Alertmanager can only return Silences with ids.

w0rm · 2018-10-18T12:43:29Z

My only concern is about introducing Maybes, why do we need them if the values are guaranteed to be there?

mxinden · 2018-10-26T12:45:16Z

My only concern is about introducing Maybes, why do we need them if the values are guaranteed to be there?

That is a valid point. I guess we have two options here:

Current situation: Have one type silence which is both used to create silences (id can not be required) as well as retrieve silences. This means, that retrieved silences would have a Maybe id property.
Use two different types. One type creatableSilence (id is not required) to create a silence. One type gettableSilence (id is always defined) to retrieve silences.

While is see the benefit of option two for the UI, reducing the noise with less Maybe operations, I would also like to point out the extra complexity for external users that now need to differentiate between the two in their client. Thereby I am voting for the first approach, having a single silence type but also having more noise in the code via Maybe handle logic.

@stuartnelson3 @w0rm What are your thoughts?

@wing328 and @trenneman from the https://github.com/OpenAPITools/openapi-generator project: Are there any best practices around this?

eriktim · 2018-10-26T13:11:10Z

Hi @mxinden, I do not know what the community's best practice is on this (maybe ask around on Elm Discourse. I often keep it as a Maybe as my application typically doesn't care about the id. Later when saving the resource I can decide whether I need to PUT or POST it, depending on the state of the Maybe. But I do not know your application and I can imagine there are use cases where it is better to split them.

This patch makes the Alertmanager UI 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>

w0rm · 2018-10-29T20:24:26Z

ui/app/src/Data/Alert.elm

+    , generatorURL : Maybe String
+    , labels : Dict String String
+    , annotations : Maybe (Dict String String)
+    , receivers : Maybe (List Receiver)


Why does Alert have so many Maybe fields? For example, what is the difference between Nothing and Just [] for receivers?

w0rm · 2018-10-29T20:33:47Z

@mxinden as far as I see it is not only about an id, but also a status of a silence. Maybe we need one type for the data, that we PUT / POST to the api, and a type of a silence that we fetch to display. Anyway, the Silence type doesn't seem like a blocker to me.

I wonder about the Alert type. It looks confusing, because almost every field is a Maybe. Is this really what the api may return?

mxinden · 2018-11-05T14:24:48Z

The only required properties of an alert are labels (@stuartnelson3 please correct me if I am wrong).

Annotations, UpdatedAt and generator URL don't need to be set in general
EndsAt doesn't need to be set e.g. when an alert is firing
StartsAt doesn't need to be set e.g. when an alert is resolved
Receivers don't need to be set when creating an alert (PUT)
Status and Fingerprint is generated server side, hence does not need to be set when creating an alert

Having an internal and an external representation of an alert, or having two different representations in the OpenAPI specification, or having multiple Maybes (current state) all adds complexity. In my opinion the laster (Maybes) is the least confusing one. What do you think @stuartnelson3 @w0rm ?

stuartnelson3 · 2018-11-05T16:50:16Z

I suppose I'm not sure. The idea that I really like about OpenAPI is that we can have a single definition file, and then generate many different language's clients from that.

From that perspective, having the additional OpenAPI definition for querying the API (one without all the maybe/optional values) would end up being much simpler, since each client would be simpler as a result.

I agree with @mxinden that it's probably less confusing to have the single definition, but I think if we can properly indicate that one of the definitions is for ingesting, and the other for consuming, the resulting clients will be much simpler to work with.

I'm not convinced one way or the other, but I am leaning towards the (hopefully) onetime creation of the second definition and supporting documentation which then results in a simpler client to work with.

w0rm

At work, we usually have different types for the update/create payload and for the get response in the Scala backend. Conceptually it may be the same entity, but the payloads and use cases differ. I don't think types are any helpful when any field can be a Maybe.

I don't want to block the progress, as I am going on vacation. But maybe we can revisit this in the future?

mxinden · 2018-11-08T09:29:30Z

I don't think types are any helpful when any field can be a Maybe.

@w0rm I think you have a very good point.

How about I split this PR up into two so that we can move forward with parts of this patch:

Move status and silences to API v2 (as is in this PR)
Split Alert type into GettableAlert and PostableAlert (better name suggestions welcome)

What do you think?

w0rm · 2018-11-08T11:37:28Z

@mxinden sounds good to me! As for naming, I prefer a simple Alert for the gettable alert and something different for creating an alert, maybe AlertCreate.

stuartnelson3 · 2018-11-09T10:52:47Z

This compromise sounds good to me 👍

mxinden · 2018-11-09T12:03:04Z

Closing here in favour of #1613.

stuartnelson3 reviewed Sep 26, 2018

View reviewed changes

ui/app/src/Data/Alert.elm Show resolved Hide resolved

stuartnelson3 reviewed Sep 26, 2018

View reviewed changes

ui/app/src/Request/Alert.elm Outdated Show resolved Hide resolved

stuartnelson3 reviewed Sep 26, 2018

View reviewed changes

mxinden mentioned this pull request Oct 17, 2018

WIP: Elm OAS3 CI tests OpenAPITools/openapi-generator#1144

Closed

8 tasks

mxinden force-pushed the api-v2-ui branch 5 times, most recently from 840236d to 8786fd0 Compare October 18, 2018 09:58

mxinden requested review from w0rm and stuartnelson3 October 18, 2018 11:53

w0rm reviewed Oct 18, 2018

View reviewed changes

mxinden force-pushed the api-v2-ui branch 2 times, most recently from bd221ab to 963986e Compare October 26, 2018 12:34

mxinden changed the title ~~[WIP] ui: Move UI to API v2~~ ui: Move UI to API v2 Oct 26, 2018

mxinden force-pushed the api-v2-ui branch from 963986e to 9858761 Compare October 26, 2018 12:53

mxinden force-pushed the api-v2-ui branch 3 times, most recently from e95bf61 to 80853e2 Compare October 26, 2018 13:22

ui: Move UI to API v2

e9e3908

This patch makes the Alertmanager UI 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>

mxinden force-pushed the api-v2-ui branch from 80853e2 to e9e3908 Compare October 26, 2018 13:42

w0rm reviewed Oct 29, 2018

View reviewed changes

w0rm approved these changes Nov 7, 2018

View reviewed changes

mxinden closed this Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Move UI to API v2 #1560

ui: Move UI to API v2 #1560

mxinden commented Sep 26, 2018 •

edited

Loading

stuartnelson3 Sep 26, 2018

stuartnelson3 Sep 26, 2018

stuartnelson3 left a comment

mxinden commented Sep 26, 2018

mxinden commented Sep 28, 2018

mxinden commented Oct 18, 2018

mxinden commented Oct 18, 2018

w0rm Oct 18, 2018

mxinden Oct 18, 2018

w0rm Oct 18, 2018

w0rm Oct 18, 2018

mxinden Oct 26, 2018

w0rm commented Oct 18, 2018

mxinden commented Oct 26, 2018

eriktim commented Oct 26, 2018

w0rm Oct 29, 2018 •

edited

Loading

w0rm commented Oct 29, 2018

mxinden commented Nov 5, 2018 •

edited by stuartnelson3

Loading

stuartnelson3 commented Nov 5, 2018

w0rm left a comment

mxinden commented Nov 8, 2018

w0rm commented Nov 8, 2018

stuartnelson3 commented Nov 9, 2018

mxinden commented Nov 9, 2018

		-}


		module Data.InlineResponse200 exposing (InlineResponse200, inlineResponse200Decoder, inlineResponse200Encoder)


		Nothing ->
		-- TODO: What to return here? This case should never happen.

ui: Move UI to API v2 #1560

ui: Move UI to API v2 #1560

Conversation

mxinden commented Sep 26, 2018 • edited Loading

stuartnelson3 Sep 26, 2018

Choose a reason for hiding this comment

stuartnelson3 Sep 26, 2018

Choose a reason for hiding this comment

stuartnelson3 left a comment

Choose a reason for hiding this comment

mxinden commented Sep 26, 2018

mxinden commented Sep 28, 2018

mxinden commented Oct 18, 2018

mxinden commented Oct 18, 2018

w0rm Oct 18, 2018

Choose a reason for hiding this comment

mxinden Oct 18, 2018

Choose a reason for hiding this comment

w0rm Oct 18, 2018

Choose a reason for hiding this comment

w0rm Oct 18, 2018

Choose a reason for hiding this comment

mxinden Oct 26, 2018

Choose a reason for hiding this comment

w0rm commented Oct 18, 2018

mxinden commented Oct 26, 2018

eriktim commented Oct 26, 2018

w0rm Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

w0rm commented Oct 29, 2018

mxinden commented Nov 5, 2018 • edited by stuartnelson3 Loading

stuartnelson3 commented Nov 5, 2018

w0rm left a comment

Choose a reason for hiding this comment

mxinden commented Nov 8, 2018

w0rm commented Nov 8, 2018

stuartnelson3 commented Nov 9, 2018

mxinden commented Nov 9, 2018

mxinden commented Sep 26, 2018 •

edited

Loading

w0rm Oct 29, 2018 •

edited

Loading

mxinden commented Nov 5, 2018 •

edited by stuartnelson3

Loading