On 10.12.2012 14:34, Petr Blaho wrote:
On Fri, Nov 30, 2012 at 11:43:19AM +0100, Jiří Stránský wrote:
> On 30.11.2012 01:18, Richard Su wrote:
>> On 11/29/2012 08:33 AM, Jiří Stránský wrote:
>>> Hi,
>>>
>>> I'd like to change some stuff in the deployments API draft and I'd
like some ACKs and/or comments on the numbered issues below. I know this API already went
around once in a RFC and sort of passed, but I think we missed things in there and
didn't decide others. I'd like to get the implementation right so that we
don't have to change it later :) The current draft is on wiki, the rest of this mail
refers to this draft [1].
>>>
>>> 1) <owner_id>
>>>
>>> *I wouldn't show owner information at all for now.* In an ideal case,
this would be a proper link to a "user" resource, but we don't have API to
access user information now. I think there's not much point in showing just an ID. We
can add the <user> link when we have the "user" resource implemented.
>>>
>>
>> Good point, I think you're pointing out the fact that we do need to implement
the user resource. As a first pass perhaps we can make it read only with basic information
like name, email, but not password. We can leave owner_id out for now until that resource
has been implemented.
>
> Hmm... not keen on this. I'm more in favour of "if you know you'll have
to change it later, don't put it there" when it comes to APIs. Adding stuff is ok
for backwards compatibility, but changing/removing is not (I think both Deltacloud and
CIMI follow this principle). With showing just <owner_id>, we don't give the
client meaningful information, but we're commiting ourselves to having to break the
backward compatibility later on (when we change it into <user>). The implementation
of <owner_id> is now just a one line of code + one line of spec I think, but I'd
like to leave it out anyway because I think it would cause more trouble than benefit.
We can include <user id="1" /> and add href attribute later when Users
API will be there. This way we have user id there and we will just add
new things to API, not changing existing.
Yes this would be possible. However, there's also a question of how to show
relationship semantics. For the first time I think, we hit the issue in API that the name
of the attribute is different from the resource name. The attribute in question is not
called "user", it is called "owner". I'd kinda like to indicate
this somehow in the response for two reasons:
* Simply to make clear what the relationship means. It's the same reason why our data
model has the relationship named "owner", not "user". I believe this
information shouldn't get lost.
* In case there are multiple users linked to a deployment (or sth else) in the future,
we'd *have* to indicate semantics to avoid ambiguity of having two <user>
links.
Maybe this could be indicated by <user rel="owner" ... />. I'm not
sure of this and it's not a priority question right now, but imho it's a reason
for not putting the <user> there at all for now.
[snip]
>>> 4) <deployable-xml>
>>>
>>> *Should we wrap the deployable template into CDATA or not?* The benefit is
that the inner XML will not be parsed by the client when parsing the API response, so an
error in the template XML can't break the whole API response. And it is semantically
cleaner, because the template XML won't be part of the API response XML tree, but will
be treated as data, which it is. (Think if we provided JSON API, then the template XML
would be treated as a data too and not converted into a corresponding JSON structure,
I'd say.) The drawback is that our deployable then can't have a CDATA in it,
because CDATA nesting is not allowed. For deployable templates, this might not be a
problem right now, but I wonder about the future. E.g. for image templates it would be a
problem [3]. So I'm sort of on the fence here, maybe a bit in favour of not wrapping
it into CDATA. (Btw Tim solves this by having image template as a separate resource, but
I'm not sure we should go this way for deployables !
as!
> we!
>>> ll.)
>>
>> I would vote for a separate resource. There is xml embedded in the instance
resource too, and this sounds like the cleanest way around it.
>
> To brainstorm further - if we make it a separate resource, I wonder if we should make
it totally separate, like Tim has it (template can be created independently of images).
Say:
>
> /api/deployabletemplates/:id
>
> Then deployables and deployments DB records would link to these templates, and
templates would have to become read-only (create, but never update), like in Tim.
>
> This would force us to do significant changes in the DB model as well (and I wonder
if we'd want/need to change UI workflow anyhow), because currently the template is
embedded as an attribute into deployable and deployment. This solution would be probably
very time consuming to implement, but should be possible.
>
> Or, if we should make it as a simple nested subresource. Say:
>
> /api/deployables/:deployable_id/template
> /api/deployment/:deployment_id/template
>
> This approaich is imho possible without performing a major surgery on Conductor and
solves the problem of API representation. The first approach on the other hand seems a bit
cleaner by design.
>
> I cannot decide right now which one I'd prefer more :)
>
I am for separated resources too.
Hehe and Richard just wrote that we should embed :) Anyways, yesterday I found that CDATA
sections *can* be nested [1], even though it's more of a trick than a designed way.
So, I think we could use CDATA for embedding XML after all.
CDATA pros:
* Seems to be the correct way (compared to non-CDATA embedding) when we treat templates as
data, not as resources
CDATA cons:
* Tim doesn't use it, Tim allows embedding templates without CDATA [2] -> we would
be inconsistent
* We would force clients to use the trick [1] in their POST/PUT requests if their
templates contained CDATA.
A problem I think of with nested resources: Even if we make it nested resource for GET
requests, we'd still have to support embedding in POST for *deployables* (deployable
has to be created with it's template in the current data model). It doesn't remove
embedding completely.
Unless I missed something important, I'm tempted to go the way of simple embedding
without CDATA, as Richard suggested.
Take care,
J.
[1]
http://en.wikipedia.org/wiki/CDATA#Nesting
[2]
https://github.com/aeolus-incubator/tim#create-base-image-with-template-a...