Merge lp:~gmb/maas/fix-null-when-releasing-ip-bug-1383668 into lp:~maas-committers/maas/trunk

[Graham Binns]

maas cli now prints a "success" message when a response comes back with a status code of 200.

Previously, only the response content would be printed, which could be confusing.

[Gavin Panella (allenap)]

I'm not sure we should do this. The API is for use by other programs.
One of those programs is the maas CLI. This message is being changed for
the benefit of the human behind the CLI, but the CLI can also be driven
by another program, and the API is available to any program.

In this exact case the script can check the return code, where HTTP 200
OK means all's well. However, this change would set a precedent that I
don't think we should set: we can't always replace the machine-readable
output with a human-readable message. Having places where the response
is for humans and others where it's for machines would give MAAS an even
more inconsistent API.

Can I suggest a few options?

1. Do all the translation from machine to human in the CLI.

2. If the human message is short, we could add it as a header to the
   response.

3. Return a multipart response with a machine and human part. This would
   cause a break in compatibility, but it's an easy one to implement at
   least.

What do you think?

review: Needs Information

[Graham Binns (gmb)]

> I'm not sure we should do this. The API is for use by other programs.
> One of those programs is the maas CLI. This message is being changed for
> the benefit of the human behind the CLI, but the CLI can also be driven
> by another program, and the API is available to any program.
>
> In this exact case the script can check the return code, where HTTP 200
> OK means all's well. However, this change would set a precedent that I
> don't think we should set: we can't always replace the machine-readable
> output with a human-readable message. Having places where the response
> is for humans and others where it's for machines would give MAAS an even
> more inconsistent API.

I see your argument — and as I've said elsewhere, the CLI needs a good coat of looking at in the next cycle (for preference). I think we should find a quick solution to your concerns here so that we can fix niggling API/CLI bugs like this more easily; otherwise we're just going to end up pushing them back for feature work.

One counter-argument is that we already send human-readable output for error cases… why is doing it for a success case setting a bad precedent, then? Surely things that are calling the API know what an API method is or isn't going to return? I do see you point, however, that the API shouldn't necessarily be the place to set success messages.

> Can I suggest a few options?
>
> 1. Do all the translation from machine to human in the CLI.
  I'm not familiar enough with the CLI code to know how big a task that is. TBH, I didn't realise that this branch would be precedent-setting.

> 2. If the human message is short, we could add it as a header to the
> response.
  Okay. This has the least impact as far as I can tell.

> 3. Return a multipart response with a machine and human part. This would
> cause a break in compatibility, but it's an easy one to implement at
> least.
  That works, but API compatibility (as we've seen today) is a big issue for some users.

[Gavin Panella (allenap)]

> One counter-argument is that we already send human-readable output for
> error cases… why is doing it for a success case setting a bad
> precedent, then?

I think we should seek to change error responses to be machine-readable.
Sometimes errors are just a bug in MAAS and there's not much we can do
but leak on the floor. Other times they're things that an API client can
use to make a decision. If the response is not machine-readable, then
the client has to take a punt on what to do next, based on substring
matching or something equally flaky.

Even a bug-in-MAAS kind of failure might deserve a machine-readable
response, for example with an OOPS number to report.

> Surely things that are calling the API know what an API method is or
> isn't going to return? I do see you point, however, that the API
> shouldn't necessarily be the place to set success messages.

I don't think it's a bad place necessarily, but I think the API needs to
be usable by machines first. The API can include hints to improve the
human experience, but not at the former's expense.

>
> > Can I suggest a few options?
> >
> > 1. Do all the translation from machine to human in the CLI. I'm not
> familiar enough with the CLI code to know how big a task
> that is. TBH, I didn't realise that this branch would be
> precedent-setting.

That's feature work for sure.

>
> > 2. If the human message is short, we could add it as a header to the
> > response.
> Okay. This has the least impact as far as I can tell.

Agreed. Some more suggestions:

* Add the HTTP header with an X-MAAS- prefix.

* Don't document it as generally useful just yet. Or if you do document
  it, explicitly state that it's experimental.

* Only print it out from the maas CLI when sys.stdout.isatty() is True.
  Perhaps present it like so:

      $ maas thing verb param=value
      Thing has been successfully verbed.
      Machine-readable output follows:
      null

  The first 2 lines would not be printed when piping to a script, for
  example.

I'll try to stop meddling now.

>
> > 3. Return a multipart response with a machine and human part. This
> > would cause a break in compatibility, but it's an easy one to
> > implement at least.
> That works, but API compatibility (as we've seen today) is a big
> issue for some users.

We'd have to bump the API version and continue to provide the old API.
However, the old API could be derived from the new, so we wouldn't break
compatibility with existing clients.

However, this is feature-scale work too.

[Julian Edwards (julian-edwards)]

Why not just make the cli print out "Success" when the returned code is a 200
and there's no other info in the content body?

[Graham Binns (gmb)]

On 22 October 2014 00:51, Julian Edwards <email address hidden> wrote:
> Why not just make the cli print out "Success" when the returned code is a 200
> and there's no other info in the content body?

Because that's just too simple and we could overthink a plate of beans?

That's a nice fix for this particular bug. It doesn't solve the
general point that Gavin's making. However, I'm all for a quick fix
for this (it's marked trivial, after all) and then onto other things.

[Gavin Panella (allenap)]

> On 22 October 2014 00:51, Julian Edwards wrote:
> > Why not just make the cli print out "Success" when the returned code
> > is a 200 and there's no other info in the content body?
>
> Because that's just too simple and we could overthink a plate of
> beans?
>
> That's a nice fix for this particular bug. It doesn't solve the
> general point that Gavin's making. However, I'm all for a quick fix
> for this (it's marked trivial, after all) and then onto other things.

Actually, I like that approach. I think it should be treated the same as
the header:

> * Only print it out from the maas CLI when sys.stdout.isatty() is
> True. Perhaps present it like so:
>
> $ maas thing verb param=value
> Thing has been successfully verbed.
> Machine-readable output follows:
> null
>
> The first 2 lines would not be printed when piping to a script, for
> example.

[Graham Binns (gmb)]

On 22 October 2014 09:30, Gavin Panella <email address hidden> wrote:
> Actually, I like that approach. I think it should be treated the same as
> the header:

+1. Though this isn't quite what Jools is suggesting:

> On 22 October 2014 00:51, Julian Edwards wrote:
> > Why not just make the cli print out "Success" when the returned code
> > is a 200 and there's no other info in the content body?

Because in this case there *is* info in the content body — a "null".
So I vote for just printing "Success\nMachine-readable output
follows:\n<CONTENT>" as you suggest.

[Gavin Panella (allenap)]

Looks good!

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 22 Oct 2014 08:51:22 you wrote:
> On 22 October 2014 09:30, Gavin Panella <email address hidden> wrote:
> > Actually, I like that approach. I think it should be treated the same as
>
> > the header:
> +1. Though this isn't quite what Jools is suggesting:
> > On 22 October 2014 00:51, Julian Edwards wrote:
> > > Why not just make the cli print out "Success" when the returned code
> > > is a 200 and there's no other info in the content body?
>
> Because in this case there *is* info in the content body — a "null".
> So I vote for just printing "Success\nMachine-readable output
> follows:\n<CONTENT>" as you suggest.

Oh I hadn't realised that text was getting sent, I presumed it was a json
artifact. Let's make it stop sending those back, they're useless.

[Graham Binns (gmb)]

> Oh I hadn't realised that text was getting sent, I presumed it was a json
> artifact. Let's make it stop sending those back, they're useless.

Well, they're a JSON artefact insofar as when the API method returns None it gets converted to u'null', and that becomes the response body.

I can't think of a situation (right now) when we'd want to return None deliberately, but I'm leery of getting rid of it, just-in-case.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~gmb/maas/fix-null-when-releasing-ip-bug-1383668 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [128 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [88.2 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [343 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [215 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Fetched 834 kB in 3s (264 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi python-openssl python-paramiko python-pexpect python-pip python-pocke...

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2014-09-04 20:01:15 +0000
+++ src/maascli/api.py	2014-10-22 10:24:18 +0000
@@ -280,9 +280,17 @@
 
         If the response is textual, a trailing \n is appended.
         """
-        file.write(content)
-        if is_response_textual(response) and file.isatty():
-            file.write("\n")
+        if file.isatty():
+            if is_response_textual(response) and response.status == 200:
+                file.write("Success.\n")
+                file.write("Machine-readable output follows:\n")
+
+            file.write(content)
+
+            if is_response_textual(response):
+                file.write("\n")
+        else:
+            file.write(content)
 
 
 class ActionHelp(argparse.Action):

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2014-09-12 03:33:27 +0000
+++ src/maascli/tests/test_api.py	2014-10-22 10:24:18 +0000
@@ -264,6 +264,7 @@
         # to a TTY, print_response() prints the response with a trailing
         # \n.
         response = httplib2.Response({
+            'status': httplib.NOT_FOUND,
             'content': "Lorem ipsum dolor sit amet.",
             'content-type': 'text/unicode',
             })
@@ -277,6 +278,7 @@
         # connected to a TTY, print_response() prints the response
         # without a trailing \n.
         response = httplib2.Response({
+            'status': httplib.FOUND,
             'content': "Lorem ipsum dolor sit amet.",
             'content-type': 'text/unicode',
             })
@@ -297,6 +299,23 @@
         api.Action.print_response(response, response['content'], buf)
         self.assertEqual(response['content'], buf.getvalue())
 
+    def test_print_response_prints_textual_response_with_success_msg(self):
+        # When the response has a status code of 200, and the response
+        # body is textual print_response() will print a success message
+        # to the TTY.
+        response = httplib2.Response({
+            'status': httplib.OK,
+            'content': "Lorem ipsum dolor sit amet.",
+            'content-type': 'text/unicode',
+            })
+        buf = io.StringIO()
+        self.patch(buf, 'isatty').return_value = True
+        api.Action.print_response(response, response['content'], buf)
+        self.assertEqual(
+            "Success.\n"
+            "Machine-readable output follows:\n" +
+            response['content'] + "\n", buf.getvalue())
+
 
 class TestActionHelp(MAASTestCase):