Merge into trunk : utopic-update-dependencies : Code : juju-quickstart

Status:

Merged

Merged at revision:

98

Proposed branch:

lp://qastaging/~frankban/juju-quickstart/utopic-update-dependencies

Merge into:

lp://qastaging/juju-quickstart

Diff against target:

203 lines (+56/-30)

6 files modified

Makefile (+1/-1)
quickstart/juju.py (+22/-11)
quickstart/tests/helpers.py (+5/-2)
quickstart/tests/test_juju.py (+22/-10)
requirements.pip (+4/-4)
test-requirements.pip (+2/-2)

To merge this branch:

bzr merge lp://qastaging/~frankban/juju-quickstart/utopic-update-dependencies

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2014-09-08	Pending
Review via email: mp+233770@code.qastaging.launchpad.net

Description of the change

Updated quickstart dependencies for utopic.

Quickstart now uses the dependency versions we
expect to be available in utopic.

Update the code parts in which quickstart interacts
with the juju client and the websocket Python
libraries: ensure we only log the textual WebSocket
messages we are interested in.

Also remove the deprecated --use-mirrors pip option.

Tests: `make check`

QA:
Use quickstart as usual to bootstrap local, ec2 and HP
environments: `.venv/bin/python juju-quickstart -e ...`

https://codereview.appspot.com/139350043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-09-08:

#

Download full text (8.8 KiB)

Reviewers: mp+233770_code.launchpad.net,

Message:
Please take a look.

Description:
Updated quickstart dependencies for utopic.

Quickstart now uses the dependency versions we
expect to be available in utopic.

Update the code parts in which quickstart interacts
with the juju client and the websocket Python
libraries: ensure we only log the textual WebSocket
messages we are interested in.

Also remove the deprecated --use-mirrors pip option.

Tests: `make check`

QA:
Use quickstart as usual to bootstrap local, ec2 and HP
environments: `.venv/bin/python juju-quickstart -e ...`

https://code.launchpad.net/~frankban/juju-quickstart/utopic-update-dependencies/+merge/233770

(do not edit description out of merge proposal)

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

Affected files (+58, -30 lines):
   M Makefile
   A [revision details]
   M quickstart/juju.py
   M quickstart/tests/helpers.py
   M quickstart/tests/test_juju.py
   M requirements.pip
   M test-requirements.pip

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2014-04-07 09:03:11 +0000
+++ Makefile 2014-09-05 14:54:31 +0000
@@ -23,7 +23,7 @@

  $(VENV_ACTIVATE): test-requirements.pip requirements.pip
   virtualenv --distribute -p $(PYTHON) $(VENV)
- $(VENV)/bin/pip install --use-mirrors -r test-requirements.pip || \
+ $(VENV)/bin/pip install -r test-requirements.pip || \
    (touch test-requirements.pip; exit 1)
   @touch $(VENV_ACTIVATE)

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: requirements.pip
=== modified file 'requirements.pip'
--- requirements.pip 2014-06-09 20:31:01 +0000
+++ requirements.pip 2014-09-05 14:54:31 +0000
@@ -25,7 +25,7 @@
  # version of websocket-client to use. Remove websocket-client as a
dependency
  # here when moving to a newer jujuclient that correctly specifies the
  # version.
-websocket-client==0.12.0
-jujuclient==0.17.5
-PyYAML==3.10
-urwid==1.1.1
+websocket-client==0.18.0
+jujuclient==0.18.4
+PyYAML==3.11
+urwid==1.2.1

Index: test-requirements.pip
=== modified file 'test-requirements.pip'
--- test-requirements.pip 2014-06-09 20:31:01 +0000
+++ test-requirements.pip 2014-09-05 14:54:31 +0000
@@ -17,7 +17,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.

  coverage==3.7.1
-flake8==2.1.0
+flake8==2.2.3
  mock==1.0.1
-nose==1.3.1
+nose==1.3.4
  -r requirements.pip

Index: quickstart/juju.py
=== modified file 'quickstart/juju.py'
--- quickstart/juju.py 2014-06-03 20:39:11 +0000
+++ quickstart/juju.py 2014-09-08 17:49:51 +0000
@@ -19,16 +19,23 @@
from __future__ import unicode_literals

import logging
+import ssl

import jujuclient
-import ssl
import websocket

+OPCODE_TEXT = websocket.ABNF.OPCODE_TEXT
+# Define the options used to initiate the secure WebSocket connection.
+SSLOPT = {
+ 'cert_reqs': ssl.CERT_NONE,
+ 'ssl_version': ssl.PROTOCOL_TLSv1,
+}
+
+
def connect(api_u...

Reviewers: mp+233770_code.launchpad.net,

Message:
Please take a look.

Description:
Updated quickstart dependencies for utopic.

Quickstart now uses the dependency versions we
expect to be available in utopic.

Update the code parts in which quickstart interacts
with the juju client and the websocket Python
libraries: ensure we only log the textual WebSocket
messages we are interested in.

Also remove the deprecated --use-mirrors pip option.

Tests: `make check`

QA:
Use quickstart as usual to bootstrap local, ec2 and HP
environments: `.venv/bin/python juju-quickstart -e ...`

https://code.launchpad.net/~frankban/juju-quickstart/utopic-update-dependencies/+merge/233770

(do not edit description out of merge proposal)

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

Affected files (+58, -30 lines):
   M Makefile
   A [revision details]
   M quickstart/juju.py
   M quickstart/tests/helpers.py
   M quickstart/tests/test_juju.py
   M requirements.pip
   M test-requirements.pip

Index: Makefile
=== modified file 'Makefile'
--- Makefile	2014-04-07 09:03:11 +0000
+++ Makefile	2014-09-05 14:54:31 +0000
@@ -23,7 +23,7 @@

$(VENV_ACTIVATE): test-requirements.pip requirements.pip
  	virtualenv --distribute -p $(PYTHON) $(VENV)
-	$(VENV)/bin/pip install --use-mirrors -r test-requirements.pip || \
+	$(VENV)/bin/pip install -r test-requirements.pip || \
  		(touch test-requirements.pip; exit 1)
  	@touch $(VENV_ACTIVATE)

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-20140901151806-lts26kypzwfy4q4e
+New revision:  
francesco.banconi@canonical.com-20140908174951-0b386y24t3djw0c7

Index: requirements.pip
=== modified file 'requirements.pip'
--- requirements.pip	2014-06-09 20:31:01 +0000
+++ requirements.pip	2014-09-05 14:54:31 +0000
@@ -25,7 +25,7 @@
  # version of websocket-client to use.  Remove websocket-client as a  
dependency
  # here when moving to a newer jujuclient that correctly specifies the
  # version.
-websocket-client==0.12.0
-jujuclient==0.17.5
-PyYAML==3.10
-urwid==1.1.1
+websocket-client==0.18.0
+jujuclient==0.18.4
+PyYAML==3.11
+urwid==1.2.1

Index: test-requirements.pip
=== modified file 'test-requirements.pip'
--- test-requirements.pip	2014-06-09 20:31:01 +0000
+++ test-requirements.pip	2014-09-05 14:54:31 +0000
@@ -17,7 +17,7 @@
  # along with this program.  If not, see <http://www.gnu.org/licenses/>.

coverage==3.7.1
-flake8==2.1.0
+flake8==2.2.3
  mock==1.0.1
-nose==1.3.1
+nose==1.3.4
  -r requirements.pip

Index: quickstart/juju.py
=== modified file 'quickstart/juju.py'
--- quickstart/juju.py	2014-06-03 20:39:11 +0000
+++ quickstart/juju.py	2014-09-08 17:49:51 +0000
@@ -19,16 +19,23 @@
  from __future__ import unicode_literals

import logging
+import ssl

import jujuclient
-import ssl
  import websocket

+OPCODE_TEXT = websocket.ABNF.OPCODE_TEXT
+# Define the options used to initiate the secure WebSocket connection.
+SSLOPT = {
+    'cert_reqs': ssl.CERT_NONE,
+    'ssl_version': ssl.PROTOCOL_TLSv1,
+}
+
+
  def connect(api_url):
      """Return an Environment instance connected to the given API URL."""
-    connection = WebSocketConnection(
-        sslopt={'ssl_version': ssl.PROTOCOL_TLSv1})
+    connection = WebSocketConnection(sslopt=SSLOPT)
      # See the websocket.create_connection function.
      connection.settimeout(websocket.default_timeout)
      connection.connect(api_url, origin=api_url)
@@ -124,19 +131,23 @@
  class WebSocketConnection(websocket.WebSocket):
      """A WebSocket client connection."""

-    def send(self, message):
+    def send(self, payload, opcode=OPCODE_TEXT):
          """Send the given WebSocket message.

-        Overridden to add logging.
+        Overridden to add logging in the case the payload is text.
          """
-        logging.debug('API message: -->  
{}'.format(message.decode('utf-8')))
-        return super(WebSocketConnection, self).send(message)
+        if opcode == OPCODE_TEXT:
+            message = payload.decode('utf-8')
+            logging.debug('API message: --> {}'.format(message))
+        return super(WebSocketConnection, self).send(payload,  
opcode=opcode)

def recv(self):
          """Receive a message from the WebSocket server.

-        Overridden to add logging.
+        Overridden to add logging in the case the received data is text.
          """
-        message = super(WebSocketConnection, self).recv()
-        logging.debug('API message: <--  
{}'.format(message.decode('utf-8')))
-        return message
+        data = super(WebSocketConnection, self).recv()
+        if isinstance(data, bytes):
+            message = data.decode('utf-8')
+            logging.debug('API message: <-- {}'.format(message))
+        return data

Index: quickstart/tests/helpers.py
=== modified file 'quickstart/tests/helpers.py'
--- quickstart/tests/helpers.py	2014-04-23 13:08:12 +0000
+++ quickstart/tests/helpers.py	2014-09-08 17:49:51 +0000
@@ -36,8 +36,11 @@
      """
      with mock.patch('logging.{}'.format(level.lower())) as mock_log:
          yield
-    expected_calls = [mock.call(message) for message in messages]
-    mock_log.assert_has_calls(expected_calls)
+    if messages:
+        expected_calls = [mock.call(message) for message in messages]
+        mock_log.assert_has_calls(expected_calls)
+    else:
+        assert not mock_log.called, 'logging unexpectedly called'

class BundleFileTestsMixin(object):

Index: quickstart/tests/test_juju.py
=== modified file 'quickstart/tests/test_juju.py'
--- quickstart/tests/test_juju.py	2014-06-03 19:10:21 +0000
+++ quickstart/tests/test_juju.py	2014-09-08 17:49:51 +0000
@@ -21,7 +21,6 @@
  import unittest

import mock
-import ssl
  import websocket

from quickstart import juju
@@ -39,8 +38,7 @@
      def test_environment_connection(self, mock_conn):
          # A connected Environment instance is correctly returned.
          env = juju.connect(self.api_url)
-        mock_conn.assert_called_once_with(
-            sslopt={'ssl_version': ssl.PROTOCOL_TLSv1})
+        mock_conn.assert_called_once_with(sslopt=juju.SSLOPT)
          conn = mock_conn()
          conn.assert_has_calls([
              mock.call.settimeout(websocket.default_timeout),
@@ -65,7 +63,8 @@
          api_url = self.api_url
          with mock.patch('websocket.create_connection') as mock_connect:
              self.env = juju.Environment(api_url)
-        mock_connect.assert_called_once_with(api_url, origin=api_url)
+        mock_connect.assert_called_once_with(
+            api_url, origin=api_url, sslopt=juju.SSLOPT)
          # Keep track of watcher changes in the changesets list.
          self.changesets = []

@@ -325,10 +324,9 @@
      snowman = 'Here is a snowman\u00a1: \u2603'

def setUp(self):
-        with mock.patch('socket.socket') as mock_socket:
-            self.conn = juju.WebSocketConnection()
-        # Patch the socket.send() function used by the send method.
-        self.mock_send = mock_socket().send
+        self.conn = juju.WebSocketConnection()
+        # The send method calls the send_frame one.
+        self.conn.send_frame = self.mock_send = mock.Mock()
          # The recv method calls the recv_data one.
          self.conn.recv_data = self.mock_recv = mock.Mock()

@@ -345,17 +343,31 @@
              self.conn.send(self.snowman.encode('utf-8'))
          self.assertTrue(self.mock_send.called)

+    def test_send_not_text(self):
+        # Outgoing non-textual messages are not logged.
+        with helpers.assert_logs([], 'debug'):
+            self.conn.send(0x0, opcode=websocket.ABNF.OPCODE_BINARY)
+        self.assertTrue(self.mock_send.called)
+
      def test_recv(self):
          # Incoming messages are properly logged.
-        self.mock_recv.return_value = (42, 'my message')
+        self.mock_recv.return_value = (juju.OPCODE_TEXT, b'my message')
          with helpers.assert_logs(['API message: <-- my message'], 'debug'):
              self.conn.recv()
          self.mock_recv.assert_called_once_with()

def test_recv_unicode(self):
          # Incoming unicode messages are properly logged.
-        self.mock_recv.return_value = (42, self.snowman.encode('utf-8'))
+        self.mock_recv.return_value = (
+            juju.OPCODE_TEXT, self.snowman.encode('utf-8'))
          expected = 'API message: <-- {}'.format(self.snowman)
          with helpers.assert_logs([expected], 'debug'):
              self.conn.recv()
          self.mock_recv.assert_called_once_with()
+
+    def test_recv_not_text(self):
+        # Incoming non-textual messages are not logged.
+        self.mock_recv.return_value = (websocket.ABNF.OPCODE_BINARY, 0x0)
+        with helpers.assert_logs([], 'debug'):
+            self.conn.recv()
+        self.mock_recv.assert_called_once_with()

Revision history for this message

Richard Harding (rharding) wrote on 2014-09-08:

#

LGTM no qa

https://codereview.appspot.com/139350043/

Revision history for this message

Jeff Pihach (hatch) wrote on 2014-09-08:

#

LGTM qa ok!

https://codereview.appspot.com/139350043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2014-09-08:

#

*** Submitted:

Updated quickstart dependencies for utopic.

Quickstart now uses the dependency versions we
expect to be available in utopic.

Update the code parts in which quickstart interacts
with the juju client and the websocket Python
libraries: ensure we only log the textual WebSocket
messages we are interested in.

Also remove the deprecated --use-mirrors pip option.

Tests: `make check`

QA:
Use quickstart as usual to bootstrap local, ec2 and HP
environments: `.venv/bin/python juju-quickstart -e ...`

R=rharding, jeff.pihach
CC=
https://codereview.appspot.com/139350043

https://codereview.appspot.com/139350043/

juju-quickstart

Merge lp://qastaging/~frankban/juju-quickstart/utopic-update-dependencies into lp://qastaging/juju-quickstart

Commit message

Description of the change

Preview Diff

Subscribers