MAAS

Code review comment for lp:~gmb/maas/fix-null-when-releasing-ip-bug-1383668

fix-null-when-releasing-ip-bug-1383668
Merge into trunk

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-21:

> 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.

> 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.

> 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.

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.

« Back to merge proposal