Expanding controller.py with basic user functions, get_models and destroy #89

mhmoerma · Mar 16, 2017

Added several functions in controller.py:

get_user
enable_user
disable_user
destroy
change_user_password
add_user
get_models: Partial implementation, the parameters all_ and username are ignored. Isn't this handled when connecting to the controller?

Issues:

destroy: The call is executed, but it cannot remove the controller model, so the controller is never destroyed, the controller model remains in status terminating
The to_json function on Type objects does not work

Missing:
the login, logout and get_model have no matching functions in _client.py

tvansteenburgh · Mar 16, 2017

Thanks for the contribution. All of the new Controller methods need to be async def, and all the facade method calls need to be awaited. Also please add integration tests where possible.

mhmoerma · Mar 17, 2017

Added integration tests for Add-user, disable user and enable user. All the controller methods now are async and facades are awaited for.

mhmoerma · Mar 23, 2017

I've added, the add-ssh, get-ssh and remove-ssh calls, as well as the grant calls on both controllers and models. The integration tests have been expanded accordingly.

johnsca · Apr 3, 2017

johnsca requested changes Apr 3, 2017

View changes

This looks really good, thanks! A few comments below, mainly about test coverage. The merge conflict should be easy to resolve; it's just the conflicting addition of additional new tests.

I believe this is missing coverage in the tests.

I'll add it in

I believe this is missing coverage in the tests.

Indeed, I'll add it in

The facade seems to be unused.

Indeed, I will remove it

Why was this added? The CleanModel context handler ought to disconnect this for us. Did you run across an issue with it not being disconnected?

This is a mistake from my end, will remove this

Why is this test commented out?

Because currently there is no possibility to request the model access of a user using the api, so it cannot be tested. But I should be included in the test asap, when model access is callable.

mhmoerma · Apr 4, 2017

The async def test_store_resources_bundle(event_loop): test seems to fail in my local setup. Is this because some file is missing (and it will pass when making the pull request to juju master) or is it something else?
Travis log

johnsca · Apr 4, 2017

@mhmoerma It looks like you are somehow missing the tests/functional/bundle directory so it's trying (and failing) to query information about that from the charm store. Those tests passed on #102 and pass for me. Is there any chance the new bundle directory got dropped when resolving the merge conflicts?

mhmoerma · Apr 5, 2017

I don't find this directory in the current master branch?

johnsca · Apr 5, 2017

@mhmoerma Sorry, I meant tests/integration/bundle.

theblues.errors.EntityNotFound: https://api.jujucharms.com/charmstore/v5//home/travis/build/mhmoerma/python-libjuju/tests/integration/bundle/meta/any?include=bundle-machine-count&include=bundle-metadata&include=bundle-unit-count&include=bundles-containing&include=charm-config&include=charm-metadata&include=common-info&include=extra-info&include=owner&include=revision-info&include=published&include=stats&include=resources&include=supported-series&include=terms

That request should not happen because it should get caught by the is_local check in Model.deploy (you can see that the URL has an absolute local path embedded in it: /home/travis/build/mhmoerma/python-libjuju/tests/integration/bundle).

mhmoerma · Apr 6, 2017

Indeed, after a merge with the master branch, everything works as it should. I've added in all the requested changes.

mhmoerma · Apr 6, 2017

Strange, the build runs locally raw_log, but fails on the test_store_resources_charm in the libjuju CI

johnsca · Apr 6, 2017

@mhmoerma That failure is spurious. It's due to juju-solutions/layer-basic#92 and was fixed in rev 19 of the ghost charm. I'm going to merge this and update the pinned rev in the test.

johnsca · Apr 6, 2017

johnsca approved these changes Apr 6, 2017

View changes

mhmoerma · Apr 7, 2017

Awesome, thanks!

tvansteenburgh · Apr 7, 2017

@mhmoerma Really appreciate your contribution, thank you!

mhmoerma added some commits Mar 16, 2017

Added user functions to controller.py, as well as the get_models call

3095363

Merge pull request #1 from mhmoerma/master
Added user functions to controller.py, as well as the get_models call

b3d7347

tvansteenburgh referenced this pull request Mar 24, 2017
Closed
UserInfo #95

mhmoerma added some commits Mar 27, 2017

Removed await in model grant

170ec5d

Merge pull request #4 from mhmoerma/master
Removed await in model grant

503ef53

mhmoerma added some commits Apr 4, 2017

Corrected remarks

69e6e73

Typo

dc0c65a

Fixed tab error

11240aa

Typo

9360b46

Removed typo's

6e88d6a

Removed list from await expression

bf64741

Removed not needed await

943a178

Added missing import

89bf388

johnsca merged commit d23810d into juju:master Apr 6, 2017
1 check failed

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

johnsca referenced this pull request Apr 7, 2017
Closed
WIP - Adding list_models and get_model #75

@@ -207,15 +225,18 @@ def kill(self):
                  cloud = list(result.clouds.keys())[0]  # only lives on one cloud
                  return tag.untag('cloud-', cloud)
-        -    def get_models(self, all_=False, username=None):
+        +    async def get_models(self, all_=False, username=None):

@@ -1178,14 +1191,13 @@ def import_ssh_key(self, identity):
                  raise NotImplementedError()
              import_ssh_keys = import_ssh_key
-        -    def get_machines(self, machine, utc=False):
+        +    async def get_machines(self):

-        -        raise NotImplementedError()
+        +        model_facade = client.ModelManagerFacade()
+        +        model_facade.connect(self.connection)
+        +        return await list(self.state.machines.keys())

@@ -120,6 +120,7 @@
                      _deploy_in_loop(new_loop, model_name))
                  await model._wait_for_new('application', 'ubuntu')
                  assert 'ubuntu' in model.applications
+        +        await model.disconnect()

juju/python-libjuju

Expanding controller.py with basic user functions, get_models and destroy #89

mhmoerma commented Mar 16, 2017

mhmoerma added some commits Mar 16, 2017

tvansteenburgh commented Mar 16, 2017

mhmoerma added some commits Mar 17, 2017

mhmoerma commented Mar 17, 2017

mhmoerma added some commits Mar 20, 2017

mhmoerma commented Mar 23, 2017 •

Edited 1 time

mhmoerma
Mar 24, 2017

tvansteenburgh referenced this pull request Mar 24, 2017

UserInfo #95

mhmoerma added some commits Mar 27, 2017

johnsca requested changes Apr 3, 2017

View changes

mhmoerma added some commits Apr 4, 2017

mhmoerma commented Apr 4, 2017 •

Edited 1 time

mhmoerma
Apr 4, 2017

johnsca commented Apr 4, 2017

mhmoerma commented Apr 5, 2017

johnsca commented Apr 5, 2017

mhmoerma added some commits Apr 5, 2017

mhmoerma commented Apr 6, 2017

mhmoerma commented Apr 6, 2017

johnsca commented Apr 6, 2017

johnsca approved these changes Apr 6, 2017

View changes

johnsca merged commit `d23810d` into juju:master Apr 6, 2017
1 check failed

1 check failed

mhmoerma commented Apr 7, 2017

tvansteenburgh commented Apr 7, 2017

johnsca referenced this pull request Apr 7, 2017

WIP - Adding list_models and get_model #75

juju/python-libjuju

Join GitHub today

Expanding controller.py with basic user functions, get_models and destroy #89

Conversation

mhmoerma commented Mar 16, 2017

mhmoerma added some commits Mar 16, 2017

tvansteenburgh commented Mar 16, 2017

mhmoerma added some commits Mar 17, 2017

mhmoerma commented Mar 17, 2017

mhmoerma added some commits Mar 20, 2017

mhmoerma commented Mar 23, 2017 • edited Edited 1 time mhmoerma Mar 24, 2017

tvansteenburgh referenced this pull request Mar 24, 2017

mhmoerma added some commits Mar 27, 2017

johnsca requested changes Apr 3, 2017 View changes

mhmoerma added some commits Apr 4, 2017

mhmoerma commented Apr 4, 2017 • edited Edited 1 time mhmoerma Apr 4, 2017

johnsca commented Apr 4, 2017

mhmoerma commented Apr 5, 2017

johnsca commented Apr 5, 2017

mhmoerma added some commits Apr 5, 2017

mhmoerma commented Apr 6, 2017

mhmoerma commented Apr 6, 2017

johnsca commented Apr 6, 2017

johnsca approved these changes Apr 6, 2017 View changes

Hide details View details johnsca merged commit d23810d into juju:master Apr 6, 2017 1 check failed

1 check failed

mhmoerma commented Apr 7, 2017

tvansteenburgh commented Apr 7, 2017

johnsca referenced this pull request Apr 7, 2017

mhmoerma commented Mar 23, 2017 •

Edited 1 time

mhmoerma
Mar 24, 2017

johnsca requested changes Apr 3, 2017

View changes

mhmoerma commented Apr 4, 2017 •

Edited 1 time

mhmoerma
Apr 4, 2017

johnsca approved these changes Apr 6, 2017

View changes

johnsca merged commit `d23810d` into juju:master Apr 6, 2017
1 check failed