python-jujuclient

Merge lp://staging/~rharding/python-jujuclient/adjust_constraint_handling into lp://staging/~hazmat/python-jujuclient/trunk

  1. adjust_constraint_handling
  2. Merge into trunk
Proposed by Richard Harding
Status: Needs review
Proposed branch: lp://staging/~rharding/python-jujuclient/adjust_constraint_handling
Merge into: lp://staging/~hazmat/python-jujuclient/trunk
Diff against target: 211 lines (+146/-7)
5 files modified
.bzrignore (+16/-0)
README.rst (+21/-1)
jujuclient.py (+29/-4)
setup.py (+4/-2)
tests/test_environment.py (+76/-0)
To merge this branch: bzr merge lp://staging/~rharding/python-jujuclient/adjust_constraint_handling
Reviewer Review Type Date Requested Status
Kapil Thangavelu Disapprove
Review via email: mp+194176@code.staging.launchpad.net

Commit message

Adjust constraint handling to drop unknown ones.

- Adjust tests and test runner along with README updates.

Description of the change

Adjust constraint handling to drop unknown ones.

- Setup a whitelist for constraints for now until we get better messaging from the client through the deployer setup.
- Update the parse_constraints helper to drop constraints that aren't valid and perform the type casting as it was.
- Update with a new tests directory and update setup.py to be able to run python setup.py test
- Update README with some beginner hacking information
- Move the function tests to a different filename and +x it so that nose would skip it during normal test runs. It's meant to be run manually with an environment
- add a .bzrignore file

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

Line 49 of the diff has the last two characters on the line transposed ("test.s").

Since we don't really use the type of the constraints unless it is "int", perhaps a boolean instead of "int"/"str" would be more direct.

On line 178 and line 212 of the diff you could use mock.assert_called_once_with(...) instead.

Reply
lp://staging/~rharding/python-jujuclient/adjust_constraint_handling updated
20. By Richard Harding

Update per review, remember to +x the test_functional file

21. By Richard Harding

Update per review correctly this time

Revision history for this message
Richard Harding (rharding) wrote :

> Line 49 of the diff has the last two characters on the line transposed
> ("test.s").
>
> Since we don't really use the type of the constraints unless it is "int",
> perhaps a boolean instead of "int"/"str" would be more direct.
>
> On line 178 and line 212 of the diff you could use
> mock.assert_called_once_with(...) instead.

Thanks for the review and comments. Updated.

Reply
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Whitelisting and handling exceptions should be done at the application level not at the client level which is policy free. Ie this implementation already misses several supported constraints, and new ones come out all the time. It also silently strips invalid constraints with no reporting back.

review: Disapprove
Reply
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

talked with rick a bit more about the reason for this on irc which is transient fix for gui. capturing for posterity.

<hazmat> rick_h_, is there any expectation or error handling for deploys in gui?
<rick_h_> hazmat: no, because it's async we fire the deploy request off and errors don't get back right now. At least in our debugging why come bundles were failing to work yesterday
<hazmat> rick_h_, that sounds like a separate / additional issue with the guiserver deployer middleware.
<rick_h_> hazmat: +1 the guiserver bit int he deployer needs to communicate errors back to the gui server in the charm
<rick_h_> hazmat: right now, in env/gui.py it calls deploy and walks away
<hazmat> rick_h_, and a common way to communicate errors is exceptions.. so three separate issues rich exception passthrough in deployer, and handle exceptions at gui server, and ... implement error handling in the gui for bundles
<rick_h_> hazmat: yes, that's the fix we want to do. To move forward today the band-aid fix came up. If it's not ok, then we'll do something else. However, trying to get to release today/tomorrow.
<hazmat> rick_h_, the closest place to gui that's reasonable to do a hack is the guiserver deployer middleware
<rick_h_> hazmat: rgr, will move the band-aid there for now then and we'll be fixing coms/deployer as a library as follow ups post-release

Reply

Unmerged revisions

21. By Richard Harding

Update per review correctly this time

20. By Richard Harding

Update per review, remember to +x the test_functional file

19. By Richard Harding

Adjust test running, add constraint checking

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