EventGrid data spec for Microsoft.Devices/Iothub by cpjagan · Pull Request #2424 · Azure/azure-rest-api-specs

cpjagan · 2018-02-05T22:53:49Z

Adding IotHub data schema for event grid specification

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-02-05T22:54:50Z

Automation for azure-sdk-for-python

Swagger to SDK encountered an unknown error: (azure-sdk-for-python)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 274, in create_context_pr
    base=sdk_base
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 137, in get_or_create_pull
    base=base
  File "/usr/local/lib/python3.6/dist-packages/github/Repository.py", line 994, in create_pull
    return self.__create_pull_1(*args, **kwds)
  File "/usr/local/lib/python3.6/dist-packages/github/Repository.py", line 1003, in __create_pull_1
    return self.__create_pull(title=title, body=body, base=base, head=head)
  File "/usr/local/lib/python3.6/dist-packages/github/Repository.py", line 1016, in __create_pull
    input=post_parameters
  File "/usr/local/lib/python3.6/dist-packages/github/Requester.py", line 172, in requestJsonAndCheck
    return self.__check(*self.requestJson(verb, url, parameters, headers, input, cnx))
  File "/usr/local/lib/python3.6/dist-packages/github/Requester.py", line 180, in __check
    raise self.__createException(status, responseHeaders, output)
github.GithubException.GithubException: 422 {'message': 'Validation Failed', 'errors': [{'resource': 'PullRequest', 'code': 'custom', 'message': 'No commits between master and restapi_auto_eventgrid/data-plane'}], 'documentation_url': 'https://developer.github.com/v3/pulls/#create-a-pull-request'}

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 29, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 197, in rest_handle_action
    return rest_pull_close(body, github_con, restapi_repo, sdk_pr_target_repo, context_tags, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 379, in rest_pull_close
    create_context_pr(sdk_pr_target_repo, context_tags, sdk_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 283, in create_context_pr
    _LOGGER.info("Unable to manage Context PR %s:\n%s", context_pr.number, response)
UnboundLocalError: local variable 'context_pr' referenced before assignment

AutorestCI · 2018-02-05T22:56:22Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

kalyanaj

Looks good to me.

dsgouda

Few redundancies that can be reduced by consolidating a few definitions and creating properties objects for inline properties.
Also, definitions defined here are not being referenced anywhere, so not sure on the intentions behind this PR

dsgouda · 2018-02-06T00:39:31Z

+    "description":
+      "Describes the schema of the Azure IoT Hub events published to Azure Event Grid. This corresponds to the Data property of an EventGridEvent."
+  },
+  "paths": {},


So if I understand correctly, you are simply defining models but not any paths from the service. What is the intention here?

This is used to document the schemas of the various events published to EventGrid. A consumer who is consuming events from EventGrid (e.g. in an Azure function or in any webhook) can use the generated classes to deserialize the event appropriately. This is the same pattern used for other existing event schemas as well (e.g. the storage events in the eventgrid data plane directory).

Thanks for the explanation.

dsgouda · 2018-02-06T00:42:13Z

+                  "type": "object",
+                  "description":
+                    "A portion of the properties that can be written only by the application back-end, and read by the device.",
+                  "properties": {


A. We discourage use of inline definitions for complex object properties.
B. Is this odata by any chance or does your model actually have properties with special characters.

Also, you are repeating this definition again in report, better pull it out into a different definition altogether and reference it here.

Yes, the model has properties with "$" to signify that they are system properties.

If these are not odata properties, we strongly recommend using x-ms-client-name (here)
You can name the property without special characters and specify its exact name with the special character as the x-ms-client-name value

dsgouda · 2018-02-06T00:44:44Z

+  },
+  "paths": {},
+  "definitions": {
+    "IotHubDeviceCreatedEventData": {


Looks like you are sharing a bunch of properties with IotHubDeviceDeletedEventData, can you consolidate the common properties into a common definition here

dsgouda · 2018-02-06T00:45:41Z

+          "type": "object",
+          "description":
+            "Information about the device twin, which is the cloud represenation of application device metadata.",
+          "properties": {


As before, please do not define inline properties like so, create a new definition

cpjagan · 2018-02-06T22:11:30Z

@dsgouda I have pushed changes based on your comments to split into smaller definitions.

AutorestCI · 2018-02-06T22:13:23Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

dsgouda · 2018-02-07T00:31:27Z

+  "definitions": {
+    "IotHubDeviceCreatedEventData": {
+      "description": "Schema of the Data property of an EventGridEvent for an Microsoft.Devices.DeviceCreated event.",
+      "$ref" : "#/definitions/DeviceLifeCycleEventData"


The right way to do this is:

"IotHubDeviceCreatedEventData": { "properties":{ "x-ms-client-flatten": true, "$ref": "#/definitions/DeviceLifeCycleEventData", "description": "device life cycle properties." } }

I would have to use the "x-ms-client-flatten": true, everywhere I reference is it?

dsgouda · 2018-02-07T00:32:11Z

+      "description": "Schema of the Data property of an EventGridEvent for an Microsoft.Devices.DeviceDeleted event.",
+      "$ref" : "#/definitions/DeviceLifeCycleEventData"
+    },
+    "DeviceLifeCycleEventData": {


A better name here is DeviceLifeCycleEventProperties

Updated definition name for DeviceLifeCycleEvent

cpjagan · 2018-02-07T19:13:52Z

@dsgouda Pushed fixes based on your comment. Please review and let me know.

AutorestCI · 2018-02-07T19:15:59Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

AutorestCI · 2018-02-07T20:17:56Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

azuresdkciprbot · 2018-02-07T20:19:14Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/eventgrid/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

dsgouda

A couple of minor comments and still recommend usage of x-ms-client-name for property names with special characters, e.g."

 "lastUpdated": {
          "type": "string",
          "description": "The ISO8601 timestamp of the last time the properties were updated.",
          "x-ms-client-name":"$lastUpdated"
        }

dsgouda · 2018-02-07T21:15:58Z

+  "paths": {},
+  "definitions": {
+    "IotHubDeviceCreatedEventData": {
+      "description": "Schema of the Data property of an EventGridEvent for a Microsoft.Devices.DeviceCreated event.",


nit: A better description would be
"Event data for IotHubDeviceCreated event"

dsgouda · 2018-02-07T21:23:17Z

+        }
+      }
+    },
+    "DeviceTwinProperties": {


nit: Use a more meaningful name here, there already exists a definition for DeviceTwinInfo with some properties
something like TwinDeviceReportedProperties or DeviceReportedProperties based on the property name

in this context reported properties has a different meaning. So using that would only confuse the reader who understands the iot hub twin concepts. DeviceTwinProperty is the right name.

Also, TwinInfo and TwinProperties were names I picked from another swagger doc that a partner team of us already uses. I would like to be consistent with those.

I will pass on this nit.

dsgouda

LGTM pending CI builds

AutorestCI · 2018-02-07T22:20:50Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

azuresdkciprbot · 2018-02-07T22:22:16Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/eventgrid/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

AutorestCI · 2018-02-07T22:41:17Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

EventGrid data spec for Microsoft.Devices/Iothub

09ac6b9

azuresdkci assigned dsgouda Feb 5, 2018

AutorestCI mentioned this pull request Feb 5, 2018

[AutoPR eventgrid/data-plane] EventGrid data spec for Microsoft.Devices/Iothub Azure/azure-sdk-for-python#1904

Merged

kalyanaj reviewed Feb 6, 2018

View reviewed changes

dsgouda suggested changes Feb 6, 2018

View reviewed changes

Update to break out complex objection into smaller definitions

f361f6c

dsgouda reviewed Feb 7, 2018

View reviewed changes

dsgouda suggested changes Feb 7, 2018

View reviewed changes

added x-ms-client-flatten attribute

59f3b9d

Updated definition name for DeviceLifeCycleEvent

Use allOf instead of flatten attribute

048ae64

dsgouda suggested changes Feb 7, 2018

View reviewed changes

Using x-ms-client-name for properties with $ symbol

ac94324

dsgouda approved these changes Feb 7, 2018

View reviewed changes

dsgouda merged commit 68a0d93 into Azure:master Feb 7, 2018

cpjagan deleted the eventgrid_iothub_spec branch February 7, 2018 23:44

kalyanaj mentioned this pull request Apr 30, 2018

Regenerated EventGrid DataPlane SDK from latest api specs and updated version to stable version. Azure/azure-sdk-for-net#4271

Merged

9 tasks

Conversation

cpjagan commented Feb 5, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Feb 5, 2018

Uh oh!

kalyanaj left a comment

Choose a reason for hiding this comment

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpjagan commented Feb 6, 2018

Uh oh!

AutorestCI commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpjagan commented Feb 7, 2018

Uh oh!

AutorestCI commented Feb 7, 2018

Uh oh!

AutorestCI commented Feb 7, 2018

Uh oh!

azuresdkciprbot commented Feb 7, 2018

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsgouda left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AutorestCI commented Feb 7, 2018

Uh oh!

azuresdkciprbot commented Feb 7, 2018

Uh oh!

AutorestCI commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

cpjagan commented Feb 5, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Feb 5, 2018 •

edited

Loading

dsgouda left a comment •

edited

Loading