Code review comment for lp:~frankban/juju-quickstart/utopic-update-dependencies

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_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()