Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Fixes for conjure-up #63
Conversation
johnsca
changed the title from
Implement Model.get_status
to
Fixes for conjure-up
Mar 2, 2017
petevg
reviewed
Mar 2, 2017
Tests pass. I am +1 on this, if the modified model_name string is backwards compatible.
| @@ -267,11 +273,13 @@ def https_connection(self): | ||
| username = accounts['user'] | ||
| password = accounts.get('password') | ||
| models = jujudata.models()[controller_name] | ||
| + if '/' not in model_name: | ||
| + model_name = '{}/{}'.format(username, model_name) |
johnsca
Mar 2, 2017
Member
It actually looks like this has always been the case in 2.x but we weren't using Connection.connect_model() directly before, so this is really more of a convenience that I thought was a bug before.
mitechie
Mar 6, 2017
Owner
this looks like it should be part of determining the model_name logic above. This could include a : ?
|
Most of this looks good, but I don't understand why we've added the loop param to the Connection class, since afaict it's never used. Am I missing something? |
|
@tvansteenburgh It gets passed to the websocket. Without that, the websocket events get processed by the wrong loop. I'm currently working on a functional test for this, btw. |
|
@johnsca Ah, thanks I missed that line. |
battlemidget
referenced this pull request
in conjure-up/conjure-up
Mar 2, 2017
Closed
migrate to libjuju #697
johnsca
added some commits
Mar 1, 2017
johnsca
referenced this pull request
in conjure-up/conjure-up
Mar 4, 2017
Closed
[wip] Switch from macumba to libjuju #710
|
Not sure if there are any more commits coming on this, but I'm +1 on what's here so far. |
johnsca
referenced this pull request
in conjure-up/conjure-up
Mar 6, 2017
Merged
Switch to libjuju and asyncio #713
mitechie
reviewed
Mar 6, 2017
Some questions and feedback. Let me know if you want to talk through any of it.
| @@ -36,13 +37,14 @@ class Connection: | ||
| """ | ||
| def __init__( | ||
| self, endpoint, uuid, username, password, cacert=None, | ||
| - macaroons=None): | ||
| + macaroons=None, loop=None): |
mitechie
Mar 6, 2017
Owner
Doe sthis need some help text and some usage docs updates? It looks like this changes how this can be fundamentally be used.
| @@ -267,11 +273,13 @@ def https_connection(self): | ||
| username = accounts['user'] | ||
| password = accounts.get('password') | ||
| models = jujudata.models()[controller_name] | ||
| + if '/' not in model_name: | ||
| + model_name = '{}/{}'.format(username, model_name) |
johnsca
Mar 2, 2017
Member
It actually looks like this has always been the case in 2.x but we weren't using Connection.connect_model() directly before, so this is really more of a convenience that I thought was a bug before.
mitechie
Mar 6, 2017
Owner
this looks like it should be part of determining the model_name logic above. This could include a : ?
| jujudata = JujuData() | ||
| + | ||
| + if ':' in model: | ||
| + controller_name, model_name = model.split(':') |
| @@ -326,6 +338,12 @@ def current_controller(self): | ||
| output = yaml.safe_load(output) | ||
| return output.get('current-controller', '') | ||
| + def current_model(self, controller_name=None): | ||
| + models = self.models()[controller_name or self.current_controller()] |
mitechie
Mar 6, 2017
Owner
I think this would be better as two steps vs combined like this. It's kind of unclear what's going on at first glance.
| if not is_local: | ||
| + parts = entity_id[3:].split('/') |
mitechie
Mar 6, 2017
Owner
we should update is_local to follow juju logic where it has to be a filepath starting with a '.'.
Then here we'd just do something like auto prefix with cs: and append the rest to the charmstore api?
johnsca
Mar 7, 2017
Member
Just verified that a local charm path doesn't have to start with ./ (at least, absolute paths work fine)
| + else: | ||
| + entity = await self.charmstore.entity(entity_id) | ||
| + ss = entity['Meta']['supported-series'] | ||
| + series = ss['SupportedSeries'][0] |
mitechie
Mar 6, 2017
Owner
we can let the charmstore do this right? Do we need to deal with the not series and such?
johnsca
Mar 7, 2017
Member
The series param is required by the API, and the charm store doesn't tell us what, if any, series was provided in the URL. So we have to parse the URL ourselves and if not provided, get the first supported series from the charm store data. I will refactor this a little bit, though, because it currently makes two requests when it could make one. I will also add some more comments.
| + placement=parse_placement(to), | ||
| + ) | ||
| + | ||
| + async def _deploy(self, charm_url, application, series, config, |
|
@tvansteenburgh I should have made multiple PRs but I was lazy and just kept adding to this one. If everyone is ok with this as is, I'd like to get it merged and I will be better about separate PRs for future changes. |
mitechie
requested changes
Mar 7, 2017
I'm +1 with the one request for some testing around the charm parsing logic.
| if not is_local: | ||
| + if not application_name: |
mitechie
Mar 7, 2017
Owner
There's no test changes for this application parsing stuff that I think would be useful.
johnsca
Mar 7, 2017
Member
It should be covered by https://github.com/juju/python-libjuju/pull/63/files#diff-2e7ca22cc563b1b07795f08e3ee7cf42R103 shouldn't it?
|
It looks like that tests the default expected case but if there's a slash
but no cs:, is there a test that it verifies. Basically there's a bunch of
if's through there but only the one case tested. If it's solidly tested
elsewhere I withdraw my comment but just looking it felt like a lot of
conditionals without a set of tests verifying the combinations.
…
|
mitechie
approved these changes
Mar 7, 2017
<3 ty and like how the testing caused a breakout of the method to be nice and clean.
johnsca commentedMar 1, 2017
•
Edited 1 time
-
johnsca
Mar 2, 2017
Includes:
Model().get_statusModel.connect_model()failing in 2.1 due to model names now including username inmodels.yaml