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: 137 lines (+94/-5)
2 files modified
jujuclient.py (+30/-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
Felipe Reyes (community) Needs Fixing
Tim Van Steenburgh (community) Needs Fixing
Review via email: mp+259439@code.staging.launchpad.net

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

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 :

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 :

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 :

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.

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

Addressing @freyes comments

59. By Jorge Niedbalski

- Added assertion as per @tim request

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

@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