add api response examples to the api documentation #682

MaximilianMeister · 2016-09-27T06:45:07Z

houndci-bot · 2016-09-27T06:45:15Z

crowbar_framework/app/controllers/repositories_controller.rb

-  #
-  # Provides the restful api call for
-  # Deactivate all Repositories   /utils/repositories/deactivate_all   POST   Destroys Repository DataBagItem
+  api :POST, "/utils/repositories/deactivate_all", "Dectivate all repositories. Destroys DataBagItems"


Metrics/LineLength: Line is too long. 102/100

houndci-bot · 2016-09-27T06:45:15Z

crowbar_framework/app/controllers/repositories_controller.rb

-  # Provides the restful api call for
-  # Deactivate a Repository   /utils/repositories/deactivate   POST   Destroys Repository DataBagItem
-  # required parameters: platform, arch, repo
+  api :POST, "/utils/repositories/deactivate", "Deactivate a single repository. Destroys a DataBagItem"


Metrics/LineLength: Line is too long. 103/100

houndci-bot · 2016-09-27T06:45:15Z

crowbar_framework/app/controllers/installer/installers_controller.rb

+      },
+      "errorMsg": "",
+      "successMsg": "Installation was successful. You will be redirected in a few seconds.",
+      "noticeMsg": "If you want to reinstall to get a fresh setup, please remove the following file: /var/lib/crowbar/install/crowbar-installed-ok"


Metrics/LineLength: Line is too long. 147/100

houndci-bot · 2016-09-27T06:45:15Z

crowbar_framework/app/controllers/barclamp_controller.rb

+    "manila": "Installation for Manila",
+    "ntp": "Common NTP service for the cluster. An NTP server or servers can be specified and all other nodes will be clients of them.",
+    "ceph": "Distributed object store and file system",
+    "network": "Instantiates network interfaces on the crowbar managed systems. Also manages the address pool",


Metrics/LineLength: Line is too long. 111/100

houndci-bot · 2016-09-27T06:45:16Z

crowbar_framework/app/controllers/barclamp_controller.rb

+    "database": "Installation for Database",
+    "horizon": "User Interface for OpenStack projects (code name Horizon)",
+    "manila": "Installation for Manila",
+    "ntp": "Common NTP service for the cluster. An NTP server or servers can be specified and all other nodes will be clients of them.",


Metrics/LineLength: Line is too long. 136/100

houndci-bot · 2016-09-27T06:45:16Z

crowbar_framework/app/controllers/barclamp_controller.rb

+    "rabbitmq": "Installation for rabbitmq",
+    "nova": "installs and configures the Openstack Nova component. It relies upon the network and glance barclamps for normal operation.",
+    "glance": "Glance service (image registry and delivery service) for the cloud",
+    "provisioner": "The roles and recipes to set up the provisioning server and a base environment for all nodes",


Metrics/LineLength: Line is too long. 114/100

houndci-bot · 2016-09-27T06:45:16Z

crowbar_framework/app/controllers/barclamp_controller.rb

+    "ipmi": "The default proposal for the ipmi barclamp",
+    "logging": "Centralized logging system based on syslog",
+    "rabbitmq": "Installation for rabbitmq",
+    "nova": "installs and configures the Openstack Nova component. It relies upon the network and glance barclamps for normal operation.",


Metrics/LineLength: Line is too long. 138/100

MaximilianMeister · 2016-09-27T06:46:47Z

(5) #upgrade Synchronize API responses with the Angular app

rsalevsky

Can you add some line breaks to fix the hound issues. It would also help to make the code more readable.

jsuchome · 2016-09-27T09:30:00Z

crowbar_framework/app/controllers/api/crowbar_controller.rb

@@ -54,6 +77,24 @@ def maintenance

  api :GET, "/api/crowbar/repocheck", "Sanity check for Crowbar server repositories"
  api_version "2.0"
+  example '


Have we already agreed on the format of the result here?

no, but do we need to show something else besides the json response?

@jsuchome @santiph as mentioned below, we may have to define some view to show extra information about response parameters that are unclear. but please in a separate small PR

santiph

I left a couple of comments across the API documentation.

Also, I haven't seen header's details, could you please add it?
Also authentication details would be desired.

santiph · 2016-09-27T09:18:28Z

crowbar_framework/app/controllers/api/backups_controller.rb

  def index
    render json: Api::Backup.all
  end

  api :GET, "/api/crowbar/backups/:id", "Returns a specific backup"
  api_version "2.0"
+  example '
+  {


How can we download the files?

with the download endpoint

santiph · 2016-09-27T09:20:15Z

crowbar_framework/app/controllers/api/crowbar_controller.rb

@@ -19,6 +19,15 @@ class Api::CrowbarController < ApiController

  api :GET, "/api/crowbar", "Show the crowbar object"
  api_version "2.0"
+  example '
+  {
+    "version": "4.0",


Can you also document the different possible versions we'll get? What is cloud6? what's crowbar-init? what's cloud7?

cloud 6 is crowbar 3.0, cloud 7 is crowbar 4.0

Please Add that as part of the documentation

where? as a comment on that line it would be probably too long

I'd prefer a long line in the example than a comment on a github review ;)
You can also add those lines outside the json example and make it multiline...

I'm reading Apipie issues and it seems there is no better approach to represent the responses other than examples. A lot of people reference to swagger.io or raml.org, claiming they both support it...

I agree with @santiph. Github PR comments should never be considered as documentation ...

@aspiers me too :) not sure who would not ... :/

santiph · 2016-09-27T09:26:13Z

crowbar_framework/app/controllers/api/backups_controller.rb

+    "created_at": "2016-09-27T06:05:10.208Z",
+    "updated_at": "2016-09-27T06:05:10.208Z",
+    "migration_level": 20160819142156
+  }


I was expecting the file to be returned as part of the backup creation.

why that? you mean it should be downloaded right after creation? I think that would be wrong

Well, I guess we can use the id to download the backup file. Although I would expect an href > download > link attribute to get it from this response

santiph · 2016-09-27T09:42:31Z

crowbar_framework/app/controllers/api/crowbar_controller.rb

+      "ha"
+    ],
+    "upgrade": {
+      "upgrading": false,


Can you please detail the meaning of each one?

ok, I can add a comment next to it

santiph · 2016-09-27T09:43:48Z

crowbar_framework/app/controllers/api/errors_controller.rb

+      "id": 1,
+      "error": "StandardError",
+      "message": "This is an error",
+      "code": null,


Please, specify the data type of code and caller. What are we expecting to receive. And what does it mean?

added an example where you can see the datatype

santiph · 2016-09-27T09:45:06Z

crowbar_framework/app/controllers/api/upgrade_controller.rb

@@ -19,6 +19,29 @@ class Api::UpgradeController < ApiController

  api :GET, "/api/upgrade", "Show the Upgrade status object"


What's the difference with upgrade under /api/crowbar/upgrade?

just look at the example :) /api/crowbar/upgrade doesn't include the checks

So /api/upgrade also contains /api/crowbar/upgrade results? Could you add that as a comment in the example?

just ignore it for now, /api/upgrade will change this sprint anyways, as soon as we start working on https://trello.com/c/OWehKX6K/126-5-implement-a-state-machine-for-the-upgrade

Fair enough

santiph · 2016-09-27T09:48:58Z

crowbar_framework/app/controllers/barclamp_controller.rb

@@ -40,6 +40,39 @@ def controller_to_barclamp

  add_help(:barclamp_index)
  api :GET, "/crowbar", "Returns a list of string names and descriptions for all barclamps"
+  example '


Shouldn't barclamps be an attribute here?

no. why do you think that?

I'd assume barclamp is an attribute of the crowbar entity, different than Version, for instance

this is the v1 api, I think you are referring to v2 /api/crowbar

Good point. Thanks

santiph · 2016-09-27T09:53:06Z

crowbar_framework/app/controllers/barclamp_controller.rb

@@ -91,6 +131,82 @@ def transition
  add_help(:show,[:id])
  api :GET, "/crowbar/:barclamp/1.0/:id",
    "Returns a document describing the instance"
+  example '


If a generic example won't cover all barclamps fields, then we need to find a way to document it.
If this example is not the proper place for that, we must find a better place for it.

santiph · 2016-09-27T11:18:49Z

crowbar_framework/app/controllers/barclamp_controller.rb

@@ -255,6 +437,63 @@ def proposals
  api :GET, "/crowbar/:barclamp/1.0/proposals/template",
    "Returns the content of a proposal template"
  param :barclamp, String, desc: "Name of the barclamp", required: true
+  example '
+  {
+    "id": "template-dns",


A proposal template for each type of barclamp supported must be documented

that would be too much IMO, as this is just an example. does it really need to be that detailed? It would significantly bloat the LOC of the barclamp_controller

You cannot provide information through the API that is not being documented.
In other words, you cannot expect attributes or fields are being consumed by a client if they're not documented somewhere.

in an ideal world I agree, that'd be super useful. but this would be an enormous amount of documentation, just imagine a proposal alone, and then repository metadata etc. we don't have the time to do that now

You can create tech debts to track that work separately and even in a different sprint, as long as PO (Vincent) is ok with it. I'm only raising issues as I see them.

well... This is not affecting our work right now, so I can still approve it if a tech debt to track this work is created and linked

as I said there will be follow ups, I really want to avoid those huge PR's. but as this is a significant amount of work which is not necessary for the upgrade process, we have to postpone it.

the card to track this is https://trello.com/c/gs8pbS7M/1170-enhance-api-documentation

santiph · 2016-09-27T11:48:11Z

Apipie Headers example: http://stackoverflow.com/questions/27028393/how-to-generate-header-for-apipie-rails-gem
Apipie/apipie-rails#340
Apipie/apipie-rails#341

MaximilianMeister · 2016-09-27T11:56:58Z

Apipie Headers example: http://stackoverflow.com/questions/27028393/how-to-generate-header-for-apipie-rails-gem
Apipie/apipie-rails#340
Apipie/apipie-rails#341

that would be done in another PR

this was still missing somehow

santiph · 2016-09-27T12:34:10Z

crowbar_framework/app/controllers/repositories_controller.rb

+  api :POST, "/utils/repositories/activate", "Activate a single repository. Creates a DataBagItem"
+  param :platform, String, desc: "Platform of the repository", required: true
+  param :arch, String, desc: "Architecture of the repository", required: true
+  param :repo, String, desc: "Name of the repository", required: true


Missing response example

there is no response here in case of success, only 200 OK (like i think in all POST requests in crowbar, except backup create)

HTTP Codes returned needs also to be documented. Later on, you can also use http codes for specific backend errors, when described

done now in #712

MaximilianMeister · 2016-09-27T17:18:53Z

@santiph some comments are really implementation specific, like the backup create response, or would require changes there. this PR is really just to define the status quo in terms of documentation.

for the inline comments bit, I am not sure if that doesn't clutter the response examples too much. we may add some extra info below the json response, but to say it again, lets keep things simple and incrementally open smaller PR's to make the reviewers easier

vuntz · 2016-09-28T07:39:42Z

@santiph let's not use this pull request as a way to change the APIs, as otherwise, this will never get merged (and tracking this here is a very good way to make sure that we will forget about some requested changes).

This pull request is just about documenting better the current behavior of the APIs, and once we have that, we can have some good discussion about what should be changed and how.

MaximilianMeister assigned jsuchome and AbelNavarro Sep 27, 2016

houndci-bot reviewed Sep 27, 2016

View reviewed changes

rsalevsky requested changes Sep 27, 2016

View reviewed changes

MaximilianMeister mentioned this pull request Sep 27, 2016

rename storages to ceph crowbar/crowbar-ceph#71

Merged

MaximilianMeister force-pushed the apidoc branch from 63ec062 to c0c110d Compare September 27, 2016 08:50

rsalevsky approved these changes Sep 27, 2016

View reviewed changes

jsuchome reviewed Sep 27, 2016

View reviewed changes

santiph suggested changes Sep 27, 2016

View reviewed changes

MaximilianMeister added 2 commits September 27, 2016 14:11

document examples of API responses

4f00346

document the repositories controller

1fbe6ee

this was still missing somehow

MaximilianMeister force-pushed the apidoc branch from c0c110d to 1fbe6ee Compare September 27, 2016 12:12

santiph reviewed Sep 27, 2016

View reviewed changes

santiph approved these changes Sep 28, 2016

View reviewed changes

MaximilianMeister merged commit 6a4c0bf into crowbar:master Sep 28, 2016

MaximilianMeister deleted the apidoc branch September 28, 2016 13:03

		@@ -19,6 +19,29 @@ class Api::UpgradeController < ApiController

		api :GET, "/api/upgrade", "Show the Upgrade status object"

add api response examples to the api documentation #682

add api response examples to the api documentation #682

Conversation

MaximilianMeister commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximilianMeister commented Sep 27, 2016

rsalevsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiph commented Sep 27, 2016

MaximilianMeister commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximilianMeister commented Sep 27, 2016

vuntz commented Sep 28, 2016