brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: REST API: bundles + types + subtypes endpoints
Date Wed, 20 Sep 2017 17:37:49 GMT
Hi all,

For `/subtypes` versus `/types`, it's not really about code duplication. 
It's that the API is unclear because it's not obvious what the 
difference/similarity is. If I saw two endpoints like that (without 
looking at the code), I'd assume they were for different things. If I 
saw query parameters, I'd immediately understand that it's a filtering 
mechanism.

I think the right api name is `/catalog` (for what you've called 
`/types`). However, deprecating what's there and adding this at the same 
endpoint will likely be confusing.

---
There's a pressing need for `/bundles` (so a user can delete a bundle), 
whereas `/types` is a nice-to-have. Do you agree?

To me, the `/types` is a version 2 of the catalog (no matter what we 
call it).

Thinking of how best to get this into the product in a way we want to 
support for a long time, the ideal would be to mark it "beta" and have 
it versioned. Other folk (e.g. GCE) do things like `/v2beta/`. I think 
that `/v2...` is the right solution from a process, api-design and 
understandability perspective.

My feeling is we should defer the `/types`, adding it as part of a 
bigger `/v2beta` effort when we're ready to invest that time. This 
includes updating the UI and getting sufficient feedback from downstream 
projects that the new API is right.

---

To Geoff's comment about the type registry being sufficient different 
from catalog, I'm not sure about that. Personally I just stretched what 
I think of as the catalog, and thus it is a catalog v2 which supports 
more things.

I think if "catalog" disappeared, being replaced by "types", it would be 
more confusing than having a new v2 catalog that does more.

Aled


On 20/09/2017 17:37, Geoff Macartney wrote:
> My two cents -
>
> 0.  Keep /bundles API PR separate from /type and do /bundles now
> 1. While I like '/catalog' for being more meaningful than '/type', I think
> what the type registry does is sufficiently different from the classic
> catalog that it needs a new name in the API.
> 1.1. /type would be my preference to /types
> 2. I'd deprecate catalog when we introduce /v2 (see later)
> 3. I'd have said do /types now or very soon but put it under a (clearly
> beta) /v2; however I take Aled's point about clutter-free.  Not sure how
> best otherwise to indicate its beta status; possibly /beta/type?  And then
> migrate to /v2/type at the later time?
> 4. The only real use of /subtype is to have an easy way to say
> 'listApplications', 'listEntities', 'listPolicies', 'listEnrichers',
> 'listLocation'.  I don't think this merits a separate endpoint, and in
> particular I would put SubTypeApi.list into /type as
> /type/?super=whatever.  As for 'listPolicies' and friends, I would have
> that as an extra query parameter (not 'super') on /type.  I don't really
> quite know what to call it! What name should we use to distinguish between
> things that are Applications, Entities, Locations, Policies, and Enrichers.
>
> G
>
>
>
> On Wed, 20 Sep 2017 at 16:31 Robert Moss <robert.moss@cloudsoft.io> wrote:
>
>> 1) like the `/catalog` name and agree with Aled about its power to
>> communicate the idea
>> 2 & 3) run `/v1` and `/v2` in parallel, with the latter being experimental
>> and iterative until a 1.0 release
>> 4) no preference
>>
>> Robert
>>
>> On 20 September 2017 at 15:45, Alex Heneveld <
>> alex.heneveld@cloudsoftcorp.com> wrote:
>>
>>> Aled, Thomas, Mark - thanks for the good feedback.
>>>
>>> All - There are some good suggestions which I agree deserve discussion.
>>> Specific points.
>>>
>>> (1) Should /types and /bundles be top-level or under a /catalog prefix ?
>>> Thomas suggested the latter which also fits.  My reason for doing
>> top-level
>>> is simply that most REST APIs these days seem to have more top-level
>>> resources.  /catalog is not necessary and in some ways it's cleaner
>> having
>>> separate endpoints.  On the other hand the /catalog prefix is nice to
>>> orient consumers, as Aled says:  `bundles` and `types` on their own
>> aren't
>>> as self-evident.  And it means we could continue to have `POST /catalog`
>> as
>>> the main way to install.
>>>
>>> (2) Should we deprecate `/catalog` ?  Thomas suggests maybe not yet.  I
>>> think we should as equivalent functionality and more is available at the
>>> new endpoints.  (Though if we go with (1), obviously we wouldn't
>> deprecate
>>> the whole endpoint, just the old calls.).  Also worth noting:  I don't
>>> think we should remove the deprecated endpoint anytime soon however.
>>>
>>> (3) Should we switch to /v2 ?  Aled has suggested we do as the generic
>>> `types` support is a significant shift from the old more restrictive
>>> `catalog`.  I don't think we should yet, however:  I'd prefer to make
>> that
>>> move when we are ready to remove all old endpoints so v2 is clutter-free.
>>> To the extent v2 can look like a subset of v1 we make life easier for
>> users
>>> and ourselves.  Also there is significant work to add a /v2 endpoint and
>> I
>>> don't think there is benefit yet to justify this work.
>>>
>>> (4) Should `/subtypes` be an endpoint peer of `/types` ?  It has been
>> noted
>>> the same functionality as `/subtypes/entity` could be done with
>>> `/types?super=entity` (with appropriate query paramter).  My reasoning
>> for
>>> making it a separate endpoint is that this is an activity I expect people
>>> will want to do a lot, avoiding the query parameter makes for a slightly
>>> nicer API.  But it is repetition in the code.
>>>
>>> (There are some other minor issues but I think I agree with the points
>> made
>>> there.)
>>>
>>> My answers:
>>>
>>> (1) slight preference for the `/catalog` prefix
>>> (2) strong pref to deprecate old calls - they are redundant and multiple
>> is
>>> confusing
>>> (3) strong pref to stay with `/v1` for now
>>> (4) slight pref for explicit `[/catalog]/subtypes` endpoint
>>>
>>> Best
>>> Alex
>>>
>>>
>>> On 20 September 2017 at 12:38, Aled Sage <aled.sage@gmail.com> wrote:
>>>
>>>> Thanks Alex.
>>>>
>>>> As per my comment in the PR at https://github.com/apache/broo
>>>> klyn-server/pull/810#issuecomment-330824643.
>>>>
>>>> ---
>>>> TL;DR:
>>>> Given this is a big API change and given I'm suggesting a `/v2` REST
>> api
>>>> then I wanted to raise it on the list as well.
>>>>
>>>> I propose we split this PR into two. The `/bundles` part we can merge
>>>> pretty quickly. However, the `/types` and `/subtypes` is too
>>> controversial
>>>> in my opinion - it probably deserves a `/v2/` of the REST api.
>>>>
>>>> We can continue detailed discussion in the PR.
>>>>
>>>> ---
>>>> I don't want to lose the word "catalog" in the REST api - it's so good
>> at
>>>> getting across the meaning for users! The alternative `/type` is just
>> not
>>>> as good, in my opinion.
>>>>
>>>> The multiple endpoints of `/types` and `/subtypes` is confusing. I'd
>>> model
>>>> the latter as just a filter on `/type`. It would be better achieved
>> with
>>> an
>>>> additional query parameter rather than a separate endpoint.
>>>>
>>>> If designing a `/v2` REST api, we could use `/catalog` instead of
>>> `/type`.
>>>> However, it will likely take a while to get to a stable and good `/v2`
>>> api.
>>>> There are other cleanup/improvements we should probably do to the REST
>>> api
>>>> if we're releasing a new version of it (e.g. exclude the deprecated
>>> stuff,
>>>> get rid of `/locations` but figure out if we really need to support
>>>> locations from brooklyn.properties, find out from the community about
>>> other
>>>> inconsistencies or hard-to-understand parts of the api).
>>>>
>>>> The meaning of `GET /subtypes/application` looks completely different
>>> from
>>>> `GET /catalog/applications`. The latter retrieves the catalog items
>>> marked
>>>> as `template`, but the new api returns everything that implements
>>>> `Application`. Perhaps this is an opportunity to get rid of the
>> "entity"
>>>> versus "template" distinction (at least in the REST api). The original
>>>> meaning/intent of "template" has entirely changed / been misused! I
>>> believe
>>>> it was originally intended as a partially-complete YAML blueprint that
>>>> someone would retrieve via the REST api, and then modify. They'd then
>>> POST
>>>> their .yaml file to deploy their app. It has now been used as the list
>> of
>>>> apps to include in a "quick launch" view. If designing a new `/v2` api,
>>> I'd
>>>> explicitly support a "quick launch" list and would get rid of the
>>>> "template" versus "application" versus "entity" distinction in the REST
>>> api
>>>> (anywhere you can use an entity, you can use an app; anywhere you need
>> an
>>>> app then a non-app entity will be auto-wrapped in a basic-app).
>>>>
>>>> Thoughts?
>>>>
>>>> Aled
>>>>
>>>>
>>>> On 07/09/2017 17:26, Alex Heneveld wrote:
>>>>
>>>>> Hi team-
>>>>>
>>>>> As mentioned earlier, I've been working on adding bundle support to
>> the
>>>>> REST API, so we can add/remove/query bundles.  And related to this,
>> and
>>> the
>>>>> type registry, is the ability to add arbitrary types but until now
>> there
>>>>> was no way to query those, so there are endpoints for types/ and
>>>>> subtypes/.  This is in #810 [1].
>>>>>
>>>>> In brief you have:
>>>>>
>>>>> *GET /bundles* - list bundle summaries
>>>>> *POST /bundles* - add ZIP or BOM YAML
>>>>> *GET /bundles/com.acme/1.0* - get details on a specific bundle
>>>>> *DELETE /bundles/com.acme/1.0* - remove a bundle
>>>>>
>>>>> *GET /types* - list all types (optionally filter by regex or fragment)
>>>>> *GET /types/acme-entity/1.0* - get details on a specific type
>>>>> *GET /subtypes**/entity* - list all entities (optionally filter by
>> regex
>>>>> or fragment); same for many other categories
>>>>>
>>>>>
>>>>> A full list including arguments is shown in the PR.
>>>>>
>>>>> Another good thing about this besides bundle-centric management and
>>>>> deletion in particular is that it entirely replaces the "catalog/"
>>> endpoint
>>>>> allowing us to deprecate it.  I expect we'll keep it around for a
>> while
>>> as
>>>>> clients (the UI, CLI) still use it but we now have equivalent methods
>>> that
>>>>> are better aligned to how we do things with bundles.  They're also
>>> quite a
>>>>> bit faster so if you've gotten bored waiting for catalog to load this
>>>>> should help (when clients are updated).  And one final benefit, we can
>>> now
>>>>> register and explore other types eg custom task types, predicates, and
>>> more.
>>>>> One thing to note is that we have fewer and simpler REST objects using
>>>>> freeform maps where we return extended type info -- eg config on
>>> entities,
>>>>> policies, etc, sensors and effectors on entities.  I'd like to use the
>>> same
>>>>> pattern for returning data on adjunct instances so that we can support
>>>>> policies, enrichers, and feeds in a consistent way (removing
>> duplication
>>>>> there).  This should tie in with Graeme's highlights work.
>>>>>
>>>>> Follow-on work will see the CLI updated to allow `br bundle delete
>>>>> com.acme:1.0` and similar.  No immediate plans to put lots of bundle
>>> info
>>>>> into the UI as bundle devs are probably comfortable with the CLI but
>> if
>>>>> anyone would like that speak up.  I have updated UI to _show_ the
>>>>> containing bundle ([2], also needs review!).
>>>>>
>>>>> Best
>>>>> Alex
>>>>>
>>>>> [1]  https://github.com/apache/brooklyn-server/pull/810
>>>>> [2]  https://github.com/apache/brooklyn-ui/pull/48
>>>>>
>>>>>
>>>>>
>>>>> On 07/09/2017 14:58, Alex Heneveld wrote:
>>>>>
>>>>>> +1 to this, with Thomas's suggestion of a list instead of map and
>>>>>> Geoff's suggestion of doing it on adjuncts.  would be nice to have
an
>>>>>> adjunct api which lets clients treat policies, enrichers, and feeds
>> the
>>>>>> same.
>>>>>>
>>>>>> i can see this being useful for policies to record selected
>> highlights
>>>>>> of their activity so a consumer doesn't have to trawl through all
>>> activity
>>>>>> to see what a policy has done lately.  last value is a good
>> compromise
>>>>>> between having some info without trying to remember everything.
>>> sensors on
>>>>>> adjuncts could be another way -- maybe we'd move to that in future
--
>>> but
>>>>>> for now that seems overly complex.
>>>>>>
>>>>>> --a
>>>>>>
>>>>>>
>>>>>> On 07/09/2017 14:02, Thomas Bouron wrote:
>>>>>>
>>>>>>> Hi Graeme.
>>>>>>>
>>>>>>> Sounds very useful to me. Would be great to have this info properly
>>>>>>> formatted in the CLI and UI.
>>>>>>>
>>>>>>> As for the structure, I would suggest to avoid spaces in map
keys as
>>>>>>> best
>>>>>>> practice, so having either:
>>>>>>>
>>>>>>> [{
>>>>>>>      ...
>>>>>>>     "highlights": {
>>>>>>>       "lastConfirmation": {
>>>>>>>         "name": "Last Confirmation",
>>>>>>>         "description": "sdjnfvsdjvfdjsng",
>>>>>>>         "time": 12345689,
>>>>>>>         "taskId": 1345
>>>>>>>       },
>>>>>>>       ...
>>>>>>>     }
>>>>>>> }]
>>>>>>>
>>>>>>> or maybe even better, something like this:
>>>>>>> [{
>>>>>>>      ...
>>>>>>>     "highlights": [
>>>>>>>       {
>>>>>>>         "name": "Last Confirmation",
>>>>>>>         "description": "sdjnfvsdjvfdjsng",
>>>>>>>         "time": 12345689,
>>>>>>>         "taskId": 1345
>>>>>>>       },
>>>>>>>       ...
>>>>>>>     ]
>>>>>>> }]
>>>>>>>
>>>>>>> In terms of implementation, it would be useful to extend it to
other
>>>>>>> types
>>>>>>> of Brooklyn Object such as enrichers, etc. Although, it looks
like
>>> Geoff
>>>>>>> has already made the same comment/suggestion.
>>>>>>>
>>>>>>> Cheers
>>>>>>>
>>>>>>>
>>>>>>> On Thu, 7 Sep 2017 at 13:30 Graeme Miller <graeme@cloudsoft.io>
>>> wrote:
>>>>>>> Hello,
>>>>>>>> I'd like to make a change to the REST API for policies. As
this is
>> a
>>>>>>>> REST
>>>>>>>> API change I figured it would be best to flag it on the mailing
>> list
>>>>>>>> first
>>>>>>>> in case anyone has any objections.
>>>>>>>>
>>>>>>>> It would be useful when consuming this API to be able to
find out
>>> more
>>>>>>>> information about the policy. Specifically, it would be useful
to
>>> find
>>>>>>>> out
>>>>>>>> things like last action performed, last policy violation,
last
>>>>>>>> confirmation, what the triggers are etc.
>>>>>>>>
>>>>>>>> To do so, I plan to amend the REST API to include 'highlights'
for
>> a
>>>>>>>> policy. Highlights are a map of a name to a tuple of information
>>>>>>>> including
>>>>>>>> description, time and task.
>>>>>>>>
>>>>>>>> Essentially this endpoint:
>>>>>>>> "GET /applications/{application}/entities/{entity}/policies"
>>>>>>>> Will now include this:
>>>>>>>> [{
>>>>>>>>      ...
>>>>>>>>     "highlights": {
>>>>>>>>       "Last Confirmation": {
>>>>>>>>         "description": "sdjnfvsdjvfdjsng",
>>>>>>>>         "time": 12345689,
>>>>>>>>         "taskId": 1345
>>>>>>>>       },
>>>>>>>>       ...
>>>>>>>>     }
>>>>>>>> }]
>>>>>>>>
>>>>>>>> Please shout if you have any problems with this, otherwise
I'll
>>> submit
>>>>>>>> a PR
>>>>>>>> shortly with this change.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Graeme Miller
>>>>>>>>
>>>>>>>>
>>>>>


Mime
View raw message