-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP Swagger #412
base: master
Are you sure you want to change the base?
HTTP Swagger #412
Conversation
package.json
Outdated
@@ -42,6 +42,8 @@ | |||
"logops": "2.1.0", | |||
"mqtt": "3.0.0", | |||
"request": "2.88.0", | |||
"swagger-jsdoc": "^3.4.0", | |||
"swagger-ui-express": "^4.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ~ instead of ^ for dependencies (have a look to other cases in this same file ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More things:
- Swagger dependencies should be moved to devDependencies
- The command to generate the swagger doc should be added to "scripts" section in this file, so any user running "npm run swagger" (or similar) may be able to generate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, Swagger dependencies moved to devDependencies with (~) instead of (^).
We don't have any command to generate the Swagger documentation, it is generated automatically. After npm update or install
you could check it:
<server_host>:7896/api-docs
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 3e1c1f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any command to generate the Swagger documentation, it is generated automatically. After
npm update or install
you could check it:
<server_host>:7896/api-docs
The procedure to build and access to swagger documentation should be documented. For instance, with a small subsection within https://github.com/telefonicaid/iotagent-ul/blob/master/docs/usermanual.md#development-documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I have tried what you say:
$ npm update
...
$ curl localhost:7896/api-docs
curl: (7) Failed to connect to localhost port 7896: Conexión rehusada
so I understand the procedure is not exaclty that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fgalan
I think that mistake is because the IoT Agent is not running. I'm sorry, I did not tell you that. You should run the IoT Agent (https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#usage) and then make the curl request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 53b15a6
docs/usermanual.md
Outdated
<server_host>:7896/api-docs | ||
``` | ||
|
||
If you want to test the HTTP Protocol, two importants points: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this point on, is this really needed to have the swagger web working at api-docs/ ? Or do you refer to steps needed to run IOTA-UL in general?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
steps needed to run IOTA-UL in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I'd suggest removing all this part (i.e. from L559). Note that you already include a link to a document that explains how to run IOTA in L553. Repeating a long explanation about it here is a bit out of scope from my point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2808c0b
docs/usermanual.md
Outdated
|
||
For example, you could use this script: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add bash code highlight
```bash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8f66e1a
docs/usermanual.md
Outdated
where `<apiKey>` is the API Key assigned to the service and `<deviceId>` is the ID of the device. | ||
|
||
All topis published by the agent (to send a comamnd or to send configuration information) to a device are not prefixed | ||
by the protocol, in this case '/ul', just include apikey and deviceid (e.g: `/FF957A98/MydeviceId/cmd` and | ||
by the protocol, in this case '/ul', just include apikey and deviceid (e.g: `/FF957A98/MydeviceId/cmd` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 205 correct All topis => All topics
Line 207 replace '/ul' with backticks /ul
to display as monospace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8f66e1a
docs/usermanual.md
Outdated
If you want to test the HTTP Protocol, two importants points: | ||
|
||
- you should know that other services are needed (see | ||
https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#installation): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove URL from text - use a link e.g.
( see [installation](https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#installation) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8f66e1a
docs/usermanual.md
Outdated
- Orion | ||
|
||
- you need to Provisioning a Device and Provisioning a Service Group (see this tutorial | ||
https://fiware-tutorials.readthedocs.io/en/latest/iot-agent/index.html#connecting-iot-devices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove URL from text - use a link e.g.
[tutorial](https://fiware-tutorials.readthedocs.io/en/latest/iot-agent/index.html#connecting-iot-devices)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8f66e1a
fixed in 8f66e1a |
docs/usermanual.md
Outdated
### Swagger | ||
|
||
In order to run Swagger, you need to execute the IoT Agent | ||
(https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#usage) and then you can access to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing square brackets for link:
[IoT Agent](http://etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 29983d1
fixed in 29983d1 |
LGTM |
LGTM |
At the present moment (January 7th, 2019), there are some comment threads (some from @jason-fox and some from e) not yet answered/fixed so I understand the work on this PR is ongoing. @fiqare-secmotic , is my understanding correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fgalan We finished all the improvements but we commented "fixed in .." only in the end, sorry. I have updated the review. Is everything all right?
docs/usermanual.md
Outdated
### Swagger | ||
|
||
In order to run Swagger, you need to execute the | ||
[IoT Agent](https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#usage) and then you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[IoT Agent](https://github.com/telefonicaid/iotagent-ul/blob/master/docs/installationguide.md#usage) and then you can | |
IOT Agent (as explained [here](installationguide.md#usage) and then you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2808c0b
lib/bindings/HTTPBindings.js
Outdated
swaggerDefinition: { | ||
info: { | ||
title: 'IoT Agent UL2 - HTTP', // Title (required) | ||
version: '1.0.0', // Version (required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refers to the version of the IOTA? In that case, it should be obtained from version field in package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is related to Swagger documentation version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to tie the version of the swagger documentation to the version of the IOTA Agent. It would be simpler.
The only case in which makes sense to have a version for the documentation and a version for the IOTA is the one in which the same IOTA version (from a code point of view) could have different documentation versions, which I think is not realistic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of the swagger documentation has been changed to the same IOTA UL version how as requested but is important to know that this version really is the first documentation version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2164dc0
access to: | ||
|
||
``` | ||
<server_host>:7896/api-docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One doubt...
IOTA exposes two ports: one corresponding to the "southbound interface" (to which the devices send measures and command responses) and another corresponding to the "nourthbound interface" (in which the IOTA provides the provisioning API, i.e. to create a new device, etc.).
Does this /api-docs endpoint work in both ports? Or only in northbound/southbound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have developed the Swagger documentation only for the HTTP protocol (https://fiware-iotagent-ul.readthedocs.io/en/latest/usermanual/index.html#http-binding), so this version of swagger documentation (1.0.0) works only in southbound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Northbound is also HTTP-based...
However, if /api-docs only works in southbound, fine with it. I just want to confirm how it works ;)
Could you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation has been based on the sending of measurements using exclusively the HTTP protocol, so works in southbound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2164dc0
@@ -193,17 +193,18 @@ Some additional remarks regarding polling commands: | |||
MQTT is a machine-to-machine (M2M)/IoT connectivity protocol, focused on a lightweight interaction between peers. MQTT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add the following entry to CHANGES_NEXT_RELEASE:
- Add: /api-docs endpoint providing swagger-based documentation
or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2808c0b
lib/bindings/HTTPBindings.js
Outdated
info: { | ||
title: 'IoT Agent UL2 - HTTP', // Title (required) | ||
version: '2.0.0', // Version (required) | ||
description: 'This documentation explains the POST and GET requests to the route /iot/d' // Description (not required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Swagger spec states that the route is always /iot/d
, however the config.js can be overridden by using a non-standard config or applying a Docker env variable. Maybe the description should use config.getConfig().iota.defaultResource
to describe the route. It would be kind of tricky to alter the swagger definition itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 43ed851
Please have a look to the travis CI report. Several linting errors appear:
|
CHANGES_NEXT_RELEASE
Outdated
@@ -1,2 +1,3 @@ | |||
- Add: check response obj before use it handling http commands | |||
- Upgrade NodeJS version from 8.16.1 to 10.17.0 in Dockerfile due to Node 8 End-of-Life | |||
- Add: /api-docs endpoint providing swagger-based documentation of the HTTP protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After clarifying that the swagger documentation refers only to southbound (https://github.com/telefonicaid/iotagent-ul/pull/412/files#r368682255), this line should be slightly modified:
- Add: /api-docs endpoint providing swagger-based documentation of the HTTP protocol | |
- Add: /api-docs endpoint providing swagger-based documentation of the HTTP southbound interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2c34c9a
lib/bindings/HTTPBindings.js
Outdated
swaggerDefinition: { | ||
info: { | ||
title: 'IoT Agent UL2 - HTTP', // Title (required) | ||
version: '2.0.0', // Version (required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you changed version 1.0.0 to 2.0.0 in commit 2164dc0. However, this is not the inteded fix.
I think it would be a good idea to tie the version of the swagger documentation to the version of the IOTA Agent. It would be simpler.
The IOTA Agent version is taken from packages.json. Thus the solution should be something like:
var package = require('../../package.json');
...
info: {
...
version: package.version,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2c34c9a
|
||
In order to run Swagger, you need to execute the IOT Agent (as explained [here](installationguide.md#usage) and then you | ||
can access to: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add a sentence about which API is being covered by the swagger documentation. Something like this:
The swagger documentation provided at /api-docs covers the HTTP southbound interface of the agent. The northbound HTTP interface is not covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 2c34c9a
docs/usermanual.md
Outdated
|
||
### Swagger | ||
|
||
In order to run Swagger, you need to execute the IOT Agent (as explained [here](installationguide.md#usage) and then you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis CI detects the following problem regarding this line:
551:49 ✓ error Incorrect usage of the term: “IOT”, use “IoT” instead terminology
Please, fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8345090
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (at commit 8345090). Thanks for your contribution!
In order to keep the homogeneity along with all the IOTA suite, we will wait to merge this PR until a similar PR providing swagger documentation could be done in the following repositories:
- IOTA Lib, covering the northbound interface with a swagger specification
- IOTA JSON, covering the HTTP southbound interface with a swagger specificaction
Not sure about:
- ITOA LWM2M. As far as I understand, Sigfox protocol is not HTTP based. Can swagger be used to describe a non-HTTP based protocol?
- IOTA Sigfox. Can swagger be used to describe a CoAP based (as LWM2M is, as far as I know) protocol?
After merging PR #415 I'm afraid some merging conflict have arisen in this PR. Fortunatelly, the solution seems easy, detailed by @jason-fox at telefonicaid/iotagent-manager#171 (comment)
Sorry for the incoveniences |
thanks for the info! @fgalan |
This pull request contains improvements made by the Secmotic team for iotagent-ul. These improvements are part of the fiQare project, which is based on ISO 25010 to improve software quality. More info: https://fiqare.eu/
Swagger is provided for HTTP protocol in:
<server_host>:7896/api-docs
Note: npm install is needed
All test in Travis has been passed successfully and the coverage in Coveralls remains the same.