Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(152)

Issue 12770044: Initial deployer implementation.

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by frankban
Modified:
11 years, 11 months ago
Reviewers:
benji, mp+179750, gary.poster
Visibility:
Public.

Description

Initial deployer implementation. This branch includes the pieces to make the deployer work in the gui server. It does not integrate it yet, the diff is already too long with just the base logic implementd. I am sorry about the diff, but it's mostly the result of two new files (the bundles module and its tests), and it includes a lot of comments/docstrings, so it should be (hopefully) easy to follow. Since the integration will be added in the next branch, there is no QA to do, just run `make unittest` from the branch root. Thank you! https://code.launchpad.net/~frankban/charms/precise/juju-gui/guiserver-bundles-initial/+merge/179750 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+1147 lines, -11 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M revision View 1 chunk +1 line, -1 line 0 comments Download
M server/guiserver/auth.py View 1 chunk +0 lines, -4 lines 0 comments Download
A server/guiserver/bundles.py View 1 chunk +413 lines, -0 lines 12 comments Download
M server/guiserver/tests/helpers.py View 2 chunks +112 lines, -1 line 0 comments Download
A server/guiserver/tests/test_bundles.py View 1 chunk +497 lines, -0 lines 0 comments Download
M server/guiserver/tests/test_utils.py View 3 chunks +72 lines, -0 lines 0 comments Download
M server/guiserver/utils.py View 3 chunks +25 lines, -0 lines 2 comments Download
M tests/requirements.pip View 1 chunk +25 lines, -5 lines 0 comments Download

Messages

Total messages: 5
frankban
Please take a look.
11 years, 11 months ago (2013-08-12 16:52:06 UTC) #1
benji
This branch is great. Nice tests and good internal documentation. LGTM https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py File server/guiserver/bundles.py (right): ...
11 years, 11 months ago (2013-08-12 20:28:06 UTC) #2
gary.poster
I guess Tempita is coming again, eh? :-) This branch is excellently done, but I ...
11 years, 11 months ago (2013-08-13 01:25:54 UTC) #3
gary.poster
(You know I won't be around for the second review, so please do find someone ...
11 years, 11 months ago (2013-08-13 19:36:46 UTC) #4
frankban
11 years, 11 months ago (2013-08-14 12:58:30 UTC) #5
Thanks a lot to you and Benji for the fantastic reviews: I hoped
to have feedback on the design of this branch, and you gave me
great ones.

> This branch is excellently done, but I am concerned about the introduction of
a
> new calling pattern, as I mention below.  If I have not convinced you with my
> argument, we can talk: maybe you can convince me. :-)  However, I don't feel
> comfortable blessing the branch yet, with those reservations.

You convinced me :-)
I am working on the implementation of a pattern similar to the one used
in the juju-core's megawatcher. This will require some work (but hopefully
not too much) and enough changes to subdivide this deployer branch in many
sub-branches. As a consequence, a will consider this one as a prototype,
and create new cards.

> 
> Thank you!
> 
> https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py
> File server/guiserver/bundles.py (right):
> 
>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py#ne...
> server/guiserver/bundles.py:40: by all WebSocket requests;
> On 2013/08/12 20:28:06, benji wrote:
> > Design thought: if Deployer is a stateless singleton, wouldn't it be better
> > modelled as a module?
> 
> I'm not crystal clear on exactly you mean by modelled, but at least at first
> blush, +0.5 on implementing it that way if it makes sense (I guess you'd have
to
> stash the configuration values on a module global? :-/ ), and -1 requiring
that
> it be implemented that way.

I am a bit concerned about defining process executors at module level,
but I think is worth experimenting this idea, perhaps in a future branch,
in a less wild future.

>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py#ne...
> server/guiserver/bundles.py:99: 'Response': {'Status': 'completed'},
> I don't think we have any other calls that can have multiple replies for the
> same RequestId.  I don't object to the pattern in principle, but I do wonder
> about the wisdom of breaking the pattern here.  It seems like it introduces
> surprise unnecessarily.
> 
> A pattern more similar to existing usages would be to have an initial call
that
> returned a response with a unique identifier for that deployment; and then a
> second call that says "tell me when the deployment with this unique identifier
> is done."  Alternatively, it could be a "tell me when the deployment changes"
> request, which would receive notifications when the queue position changes;
> after that, the API client would have to issue another "tell me when the
> deployment changes" to learn the next status change.  That would also work
when
> we are able to send more details about the change, like steps accomplished.
> 
> This pattern matches the existing APIs (like the watcher) and so I think it
> would be a better approach.
> 
> Having an identifier for the deployment would also allow additional commands
> specific to the deployment in the future (like, "cancel" or, in the future,
> "skip the currently failing step and resume").  However, that's not really my
> argument.
> 
> Do you object to this change?  I really think it would be a better approach to
> not introduce a new, surprising pattern.

+1, working on it.

>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py#ne...
> server/guiserver/bundles.py:125: SUPPORTED_API_VERSIONS = ['go']
> So we can add support for the upcoming Clojure implementation of Juju? ;-)

Heh, I was thinking more about "go2.0", but a "clojure" version would be
definitely welcome, or a Python one! oh, wait... :-)

>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py#ne...
> server/guiserver/bundles.py:204: name, bundle = self._validate_data(data)
> It strikes me that, in the case of a queue, it would be wise to run this again
> right before attempting to process it.  Could be for later.

+1, mental note added.

>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/bundles.py#ne...
> server/guiserver/bundles.py:242: raise ValueError('invalid YAML contents')
> If there were some more detailed error message to pass through, that would be
> nice.

Good point, this must be fixed.

>
https://codereview.appspot.com/12770044/diff/1/server/guiserver/utils.py#newc...
> server/guiserver/utils.py:71: if (err.errno != errno.EEXIST) or (not
> os.path.isdir(path)):
> When would "(not os.path.isdir(path))" be true?  Might be worth a comment.

Cool.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b