Merge into trunk : do-not-show-user-prefix : Code : juju-quickstart

Status:	Merged
Merged at revision:	108
Proposed branch:	lp://qastaging/~frankban/juju-quickstart/do-not-show-user-prefix
Merge into:	lp://qastaging/juju-quickstart
Diff against target:	203 lines (+47/-22) 6 files modified quickstart/app.py (+1/-1) quickstart/juju.py (+11/-0) quickstart/models/jenv.py (+9/-7) quickstart/tests/models/test_jenv.py (+4/-3) quickstart/tests/test_app.py (+11/-11) quickstart/tests/test_juju.py (+11/-0)
To merge this branch:	bzr merge lp://qastaging/~frankban/juju-quickstart/do-not-show-user-prefix
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2014-12-16	Pending
Review via email: mp+244842@code.qastaging.launchpad.net

Description of the change

Do not output the user prefix.

QA:
run quickstart and check that the username
printed to stdout does not include the "srart-"
prefix.

https://codereview.appspot.com/187320043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-12-16:

#

Download full text (9.4 KiB)

Reviewers: mp+244842_code.launchpad.net,

Message:
Please take a look.

Description:
Do not output the user prefix.

QA:
run quickstart and check that the username
printed to stdout does not include the "srart-"
prefix.

https://code.launchpad.net/~frankban/juju-quickstart/do-not-show-user-prefix/+merge/244842

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/187320043/

Affected files (+49, -22 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/juju.py
   M quickstart/models/jenv.py
   M quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_juju.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision:
<email address hidden>

Index: quickstart/app.py
=== modified file 'quickstart/app.py'
--- quickstart/app.py 2014-12-15 17:31:23 +0000
+++ quickstart/app.py 2014-12-16 11:10:10 +0000
@@ -346,7 +346,7 @@
          else:
              break
      try:
- env.login(password, user=username)
+ env.authenticate(username, password)
      except jujuclient.EnvError as err:
          msg = 'unable to log in to the Juju API server on {}: {}'
          raise ProgramExit(msg.format(api_url, err.message))

Index: quickstart/juju.py
=== modified file 'quickstart/juju.py'
--- quickstart/juju.py 2014-11-12 08:48:35 +0000
+++ quickstart/juju.py 2014-12-16 11:10:10 +0000
@@ -25,6 +25,8 @@
import websocket

+# Define the constant tag used by Juju for user entities.
+JUJU_USER_TAG = 'user'
  OPCODE_TEXT = websocket.ABNF.OPCODE_TEXT
  # Define the options used to initiate the secure WebSocket connection.
  SSLOPT = {
@@ -50,6 +52,15 @@
      deployments to specific machines.
      """

+ def authenticate(self, username, password):
+ """Log in into the Juju environment with the given credentials.
+
+ The user name must be provided without the default Juju prefix for
+ users. Use self.login to log in specifying the complete user tag.
+ """
+ return self.login(
+ password, user='{}-{}'.format(JUJU_USER_TAG, username))
+
      def deploy_bundle(self, yaml, name=None, bundle_id=None):
          """Deploy a bundle."""
          params = {'YAML': yaml}

Index: quickstart/models/jenv.py
=== modified file 'quickstart/models/jenv.py'
--- quickstart/models/jenv.py 2014-12-15 17:31:23 +0000
+++ quickstart/models/jenv.py 2014-12-16 11:10:10 +0000
@@ -31,8 +31,6 @@
)

-# Define the constant tag used by Juju for user entities.
-JUJU_USER_TAG = 'user'
# Define the default Juju user when an environment is initially
bootstrapped.
JUJU_DEFAULT_USER = 'admin'

@@ -70,6 +68,8 @@
"""Return the Juju environment credentials stored in the jenv file.

The credentials are returned as a tuple (username, password).
+ Note that the returned user name is not prefixed with the Juju default
+ prefix for user entities ("user-).

...

Reviewers: mp+244842_code.launchpad.net,

Message:
Please take a look.

Description:
Do not output the user prefix.

QA:
run quickstart and check that the username
printed to stdout does not include the "srart-"
prefix.

https://code.launchpad.net/~frankban/juju-quickstart/do-not-show-user-prefix/+merge/244842

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/187320043/

Affected files (+49, -22 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/juju.py
   M quickstart/models/jenv.py
   M quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_juju.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:  
francesco.banconi@canonical.com-20141216092724-9bmqh1y5ak9im5gy
+New revision:  
francesco.banconi@canonical.com-20141216111010-cbdkvio8z1xl4nao

Index: quickstart/app.py
=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-12-15 17:31:23 +0000
+++ quickstart/app.py	2014-12-16 11:10:10 +0000
@@ -346,7 +346,7 @@
          else:
              break
      try:
-        env.login(password, user=username)
+        env.authenticate(username, password)
      except jujuclient.EnvError as err:
          msg = 'unable to log in to the Juju API server on {}: {}'
          raise ProgramExit(msg.format(api_url, err.message))

Index: quickstart/juju.py
=== modified file 'quickstart/juju.py'
--- quickstart/juju.py	2014-11-12 08:48:35 +0000
+++ quickstart/juju.py	2014-12-16 11:10:10 +0000
@@ -25,6 +25,8 @@
  import websocket

+# Define the constant tag used by Juju for user entities.
+JUJU_USER_TAG = 'user'
  OPCODE_TEXT = websocket.ABNF.OPCODE_TEXT
  # Define the options used to initiate the secure WebSocket connection.
  SSLOPT = {
@@ -50,6 +52,15 @@
      deployments to specific machines.
      """

+    def authenticate(self, username, password):
+        """Log in into the Juju environment with the given credentials.
+
+        The user name must be provided without the default Juju prefix for
+        users. Use self.login to log in specifying the complete user tag.
+        """
+        return self.login(
+            password, user='{}-{}'.format(JUJU_USER_TAG, username))
+
      def deploy_bundle(self, yaml, name=None, bundle_id=None):
          """Deploy a bundle."""
          params = {'YAML': yaml}

Index: quickstart/models/jenv.py
=== modified file 'quickstart/models/jenv.py'
--- quickstart/models/jenv.py	2014-12-15 17:31:23 +0000
+++ quickstart/models/jenv.py	2014-12-16 11:10:10 +0000
@@ -31,8 +31,6 @@
  )

-# Define the constant tag used by Juju for user entities.
-JUJU_USER_TAG = 'user'
  # Define the default Juju user when an environment is initially  
bootstrapped.
  JUJU_DEFAULT_USER = 'admin'

@@ -70,6 +68,8 @@
      """Return the Juju environment credentials stored in the jenv file.

The credentials are returned as a tuple (username, password).
+    Note that the returned user name is not prefixed with the Juju default
+    prefix for user entities ("user-).

Raise a ValueError if:
          - the environment file is not found;
@@ -81,13 +81,15 @@

# Retrieve the user name.
      try:
-        user = _get_value_from_yaml(data, 'user')
+        username = _get_value_from_yaml(data, 'user')
      except ValueError as err:
          # This is probably an old version of Juju not supporting multiple
          # users. Return the default user name.
-        logging.warn('cannot retrieve the user name from {}: {}'.format(
-            jenv_path, bytes(err).decode('utf-8')))
-        user = JUJU_DEFAULT_USER
+        logging.warn(
+            'cannot retrieve the user name from {}: {}: '
+            'falling back to the default user name' .format(
+                jenv_path, bytes(err).decode('utf-8')))
+        username = JUJU_DEFAULT_USER

# Retrieve the password.
      try:
@@ -104,7 +106,7 @@
          except ValueError as err:
              raise ValueError(msg + bytes(err))

-    return '{}-{}'.format(JUJU_USER_TAG, user), password
+    return username, password

def _get_value_from_yaml(data, *args):

Index: quickstart/tests/test_app.py
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-12-15 17:31:23 +0000
+++ quickstart/tests/test_app.py	2014-12-16 11:10:10 +0000
@@ -727,7 +727,7 @@
          # The user name and password are successfully retrieved.
          with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
              username, password = app.get_credentials('ec2')
-        self.assertEqual('user-admin', username)
+        self.assertEqual('admin', username)
          self.assertEqual('Secret!', password)

def test_error(self):
@@ -779,8 +779,8 @@
              env = app.connect(self.api_url, self.username, self.password)
          mock_connect.assert_called_once_with(self.api_url)
          mock_env = mock_connect()
-        mock_env.login.assert_called_once_with(
-            self.password, user=self.username)
+        mock_env.authenticate.assert_called_once_with(
+            self.username, self.password)
          self.assertEqual(mock_env, env)

@mock.patch('time.sleep')
@@ -811,8 +811,8 @@
              env = app.connect(self.api_url, self.username, self.password)
          mock_connect.assert_called_with(self.api_url)
          self.assertEqual(2, mock_connect.call_count)
-        mock_env.login.assert_called_once_with(
-            self.password, user=self.username)
+        mock_env.authenticate.assert_called_once_with(
+            self.username, self.password)
          self.assertEqual(mock_env, env)
          mock_sleep.assert_called_once_with(1)
          expected = 'unable to connect to the Juju API server on {}: bad  
wolf'
@@ -823,20 +823,20 @@
          # A ProgramExit is raised if an error occurs in the authentication.
          expected = 'unable to log in to the Juju API server on {}: bad  
wolf'
          with mock.patch('quickstart.juju.connect') as mock_connect:
-            mock_login = mock_connect().login
-            mock_login.side_effect = self.make_env_error('bad wolf')
+            mock_authenticate = mock_connect().authenticate
+            mock_authenticate.side_effect = self.make_env_error('bad wolf')
              with self.assert_program_exit(expected.format(self.api_url)):
                  app.connect(self.api_url, self.username, self.password)
          mock_connect.assert_called_with(self.api_url)
-        mock_login.assert_called_once_with(
-            self.password, user=self.username)
+        mock_authenticate.assert_called_once_with(
+            self.username, self.password)

def test_other_errors(self):
          # Any other errors occurred during the log in process are not  
trapped.
          error = ValueError('explode!')
          with mock.patch('quickstart.juju.connect') as mock_connect:
-            mock_login = mock_connect().login
-            mock_login.side_effect = error
+            mock_authenticate = mock_connect().authenticate
+            mock_authenticate.side_effect = error
              with self.assertRaises(ValueError) as context_manager:
                  app.connect(self.api_url, self.username, self.password)
          self.assertIs(error, context_manager.exception)

Index: quickstart/tests/test_juju.py
=== modified file 'quickstart/tests/test_juju.py'
--- quickstart/tests/test_juju.py	2014-09-08 17:49:51 +0000
+++ quickstart/tests/test_juju.py	2014-12-16 11:10:10 +0000
@@ -266,6 +266,17 @@
          mock_rpc.assert_called_once_with(expected)

@patch_rpc
+    def test_authenticate(self, mock_rpc):
+        # The authentication is performed with the given credentials.
+        self.env.authenticate('who', 'passwd')
+        expected = {
+            'Type': 'Admin',
+            'Request': 'Login',
+            'Params': {'AuthTag': 'user-who', 'Password': 'passwd'},
+        }
+        mock_rpc.assert_called_once_with(expected)
+
+    @patch_rpc
      def test_create_auth_token(self, mock_rpc):
          self.env.create_auth_token()
          expected = dict(Type='GUIToken', Request='Create')

Index: quickstart/tests/models/test_jenv.py
=== modified file 'quickstart/tests/models/test_jenv.py'
--- quickstart/tests/models/test_jenv.py	2014-12-15 17:31:23 +0000
+++ quickstart/tests/models/test_jenv.py	2014-12-16 11:10:10 +0000
@@ -119,7 +119,7 @@
          data = {'user': 'jean-luc', 'password': 'Secret!'}
          with self.make_jenv('local', yaml.safe_dump(data)):
              username, password = jenv.get_credentials('local')
-        self.assertEqual('user-jean-luc', username)
+        self.assertEqual('jean-luc', username)
          self.assertEqual('Secret!', password)

def test_default_user(self):
@@ -130,10 +130,11 @@
              expected_logs = [
                  'cannot retrieve the user name from {}: '
                  'invalid YAML contents: '
-                'user key not found in the root section'.format(path)]
+                'user key not found in the root section: '
+                'falling back to the default user name'.format(path)]
              with helpers.assert_logs(expected_logs, 'warn'):
                  username, password = jenv.get_credentials('local')
-        self.assertEqual('user-admin', username)
+        self.assertEqual('admin', username)
          self.assertEqual('Secret!', password)

def test_using_admin_secret(self):

Revision history for this message

Brad Crittenden (bac) wrote on 2014-12-16:

#

LGTM Francesco. Thanks.

https://codereview.appspot.com/187320043/

Revision history for this message

Richard Harding (rharding) wrote on 2014-12-16:

#

LGTM ty

https://codereview.appspot.com/187320043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-12-16:

#

Thanks for the reviews!

https://codereview.appspot.com/187320043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-12-16:

#

*** Submitted:

Do not output the user prefix.

QA:
run quickstart and check that the username
printed to stdout does not include the "srart-"
prefix.

R=bac, rharding
CC=
https://codereview.appspot.com/187320043

https://codereview.appspot.com/187320043/

juju-quickstart

Merge lp://qastaging/~frankban/juju-quickstart/do-not-show-user-prefix into lp://qastaging/juju-quickstart

Commit message

Description of the change

Preview Diff

Subscribers