python-jujuclient

Merge lp://staging/~niedbalski/python-jujuclient/lp-1456332 into lp://staging/python-jujuclient

  1. lp-1456332
  2. Merge into trunk
Proposed by Jorge Niedbalski
Status: Superseded
Proposed branch: lp://staging/~niedbalski/python-jujuclient/lp-1456332
Merge into: lp://staging/python-jujuclient
Diff against target: 138 lines (+95/-5)
2 files modified
jujuclient.py (+31/-5)
test_jujuclient.py (+64/-0)
To merge this branch: bzr merge lp://staging/~niedbalski/python-jujuclient/lp-1456332
Reviewer Review Type Date Requested Status
Tim Van Steenburgh Pending
Felipe Reyes Pending
Review via email: mp+259449@code.staging.launchpad.net

This proposal supersedes a proposal from 2015-05-18.

This proposal has been superseded by a proposal from 2015-05-19.

Description of the change

This patch fixes LP: #1456332

To post a comment you must log in.
Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote : Posted in a previous version of this proposal

One minor quibble: it's not clear what happens if you call Run without providing any machines, services, or units. Is that an error from Juju, or is it the equivalent of calling RunOnAllMachines? It would be great to clarify this in the run() docstring.

Otherwise looks good, pending a successful test run.

review: Needs Fixing
Revision history for this message
Felipe Reyes (freyes) wrote : Posted in a previous version of this proposal

Hi @niedbalski, I was reviewing your patch and looks fine to me, but I would like to ask you to add two more tests, to test when units are passed and another one for service, at the moment you're only testing with machines.

Best,

review: Needs Fixing
Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

Thanks guys for taking the time for reviewing.

@Tim,

I think that this is indeed a Juju-core validation error.

I submitted a PR for handling this on the server side: https://github.com/juju/juju/pull/2355

I don't think we should threat this as a special case, since this is a library and
the RPC call should warn about that. But, if you have any other idea, please let me know.

@Felipe,

I re-submitted addressing your comments adding 2 new tests.

Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

@Tim

I added an assertion if no target is passed, just in case.

lp://staging/~niedbalski/python-jujuclient/lp-1456332 updated
60. By Jorge Niedbalski

assert if no target is passed

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches