Added new receivers to action group document. #2077

shriku11 · 2017-12-01T22:22:18Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

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.

fearthecowboy · 2017-12-04T17:31:03Z

Trading with @marstr for a data-plane one (this looks pretty simple, so ... good trade :D )

marstr

There are some semantic errors that need to be fixed up before we go too far. :)

marstr · 2017-12-04T21:25:26Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

+                    "items": {
+                        "$ref": "#/definitions/ItsmReceiver"
+                    },
+                    "description": "The list of Itsm receivers that are part of this action group."


[nit] Because "Itsm" looks so conspicuously like "Items", in the description I would either write it out or at least have it capitalized to "ITSM" to demonstrate that it is an acronym.

marstr · 2017-12-04T21:33:38Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

@@ -459,6 +473,55 @@
                    "description": "The URI where webhooks should be sent."
                }
            },
+            "ItsmReceiver": {


I believe this is the root cause of the failures from our CI system. I think there's a curly-brace missing above this line.

marstr

There are couple of outstanding Litner errors that were introduced which I'd like to see cleaned up (find them here). Other than that, I think this PR is good to go :)

marstr · 2017-12-05T22:00:10Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

@@ -464,6 +478,55 @@
                "serviceUri"
            ]
        },
+        "ItsmReceiver": {


Because these are not required properties, I don't believe this is a breaking change.

Adding new properties - required or optional is considered a breaking change. This could result in clients unitentionally overriding the properties on the server. Imagine a resource got created with this new property. Then the GET on the resource happened from a client which did not understand this property and so ignored it. Now if the PUT happens from the same client using the resource that was obtained via GET, the value of this property on the server will be overridden.

So it is advisable to increment the api-version for this.

This is already released for this API version and is almost like a baseline now. In future we are planning to support patch operation for individual receiver types. We will change the version then.

marstr · 2017-12-05T22:11:13Z

To be clear, I'm only asking to fix the two new linter errors that are introduced with this change, not all six.

shriku11 · 2017-12-08T19:43:51Z

Ping

marstr

Thanks for making those updates, things are definitely coming together. There are still some out-lying CI errors to take care of:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/315048599#L693

In particular, it seems like there are some properties in the examples that aren't present in the swagger. Either removing them from the examples or adding them into the Swagger is pretty important.

ravbhatnagar

Please take a look at comments below

ravbhatnagar · 2017-12-14T18:56:43Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

+                    "type": "string",
+                    "description": "XML blob for the configurations of the ITSM action. CreateMultipleWorkItems option will be part of this blob as well."
+                },
+                "region": {


make this an enum

You mean region field? It is already released as string field for better readability.

ravbhatnagar · 2017-12-14T18:57:15Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

+                    "type": "string",
+                    "description": "Unique identification of ITSM connection among multiple defined in above workspace."
+                },
+                "ticketConfiguration": {


What is this property? Is this xml?

It is JSON configuration string.

Good catch, the description is fixed in new iteration.

ravbhatnagar · 2017-12-14T18:59:23Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

+                    "type": "boolean",
+                    "description": "Indicates whether this instance is global runbook."
+                },
+                "name": {


webhookResourceId property contains the name of the webhook. Why is the name needed as a separate property also?

This can be used as friendly name by user also webhookResourceId format is different with subscriptionid resourcegroup details etc.

ravbhatnagar · 2017-12-14T19:04:48Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

@@ -464,6 +478,55 @@
                "serviceUri"
            ]
        },
+        "ItsmReceiver": {


Adding new properties - required or optional is considered a breaking change. This could result in clients unitentionally overriding the properties on the server. Imagine a resource got created with this new property. Then the GET on the resource happened from a client which did not understand this property and so ignored it. Now if the PUT happens from the same client using the resource that was obtained via GET, the value of this property on the server will be overridden.

ravbhatnagar · 2017-12-14T19:05:49Z

specification/monitor/resource-manager/microsoft.insights/2017-04-01/actionGroups_API.json

@@ -464,6 +478,55 @@
                "serviceUri"
            ]
        },
+        "ItsmReceiver": {


So it is advisable to increment the api-version for this.

shriku11 · 2018-01-02T17:58:50Z

@marstr, please resolve the conflicts by over writing old files by new ones. I do not have access to resolve the conflicts.

marstr · 2018-01-03T17:20:20Z

@shriku11 says:
@marstr, please resolve the conflicts by over writing old files by new ones. I do not have access to resolve the conflicts.

I'm not exactly sure what you mean about not having access, we should work out whatever troubles you were having. This PR is based on your fork, so there shouldn't be anything you don't have access to.

That said, I have fixed the merge conflicts for you. :)

edit: I think I figured out what you were saying. Did you mean that the little "resolve conflicts" button was grey here on GitHub? If that was the trouble, it wasn't an access problem. Rather, it simply meant that the web-editor wasn't able to handle the resolution problems and that you would need to resolve them on your local machine.

azuresdkciprbot · 2018-01-03T18:16:30Z

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/monitor/resource-manager/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.

shriku11 · 2018-01-03T18:40:39Z

@marstr, I got the merge conflict button disabled so thought I do not have rights to resolve it. I also see you have changed the branch from current to master for for this PR. That resulted in conflicts as there was directory called Stable in master and not in Current branch. Also while resolving conflicts looks like unlike other files you forgot to add the patchActionGroup.json in the directory. I have added that and now getting following error, FATAL: swagger-document/loader - FAILED FATAL: Error: Could not read 'file:///git-restapi/specification/monitor/resource-manager/microsoft.insights/stable/2017-05-01-preview/diagnosticsSettings_API.json'. Any idea? Thanks, -Shripad -------------------------------------------- On Wed, 1/3/18, Martin Strobel <notifications@github.com> wrote: Subject: Re: [Azure/azure-rest-api-specs] Added new receivers to action group document. (#2077) To: "Azure/azure-rest-api-specs" <azure-rest-api-specs@noreply.github.com> Cc: "shriku11" <shriku1@yahoo.com>, "Mention" <mention@noreply.github.com> Date: Wednesday, January 3, 2018, 9:20 AM @shriku11 says: @marstr, please resolve the conflicts by over writing old files by new ones. I do not have access to resolve the conflicts. I'm not exactly sure what you mean about not having access, we should work out whatever troubles you were having. This PR is based on your fork, so there shouldn't be anything you don't have access to. That said, I have fixed the merge conflicts for you. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

marstr · 2018-01-05T19:03:15Z

@ravbhatnagar would you mind taking a look at the most recent changes to see if your comments have been addressed?

marstr · 2018-01-08T20:37:09Z

I'm needing to load-balance some of my PRs. I'm going to reassign this to @veronicagg.

ravbhatnagar · 2018-01-09T01:14:32Z

@shriku11 - Signing off on this. But we need to make sure going forward the APIs are reviewed and approved before the changes are deployed to service side. The Microsoft REST API versioning guidance should be followed when shipping changes to the service.

veronicagg · 2018-01-09T02:04:43Z

@ravbhatnagar thanks for approving from ARM side.
@shriku11 the issue is from the path in the readme file. I created and merged a PR to fix it (since it's anyway independent from this change) - #2227. Could you update this PR by pulling from master? CI should kick off again.

azuresdkciprbot · 2018-01-09T19:18:46Z

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/monitor/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

shriku11 · 2018-01-09T19:21:49Z

Merged from master after fix from @veronicagg

veronicagg

PR looks good to me.
Labeling with potential-sdk-breaking-changes, as there are new properties added on existing api-version.

load balanced review

AutorestCI · 2018-01-09T22:32:46Z

Azure/azure-sdk-for-ruby#1227

AutorestCI · 2018-01-09T22:33:35Z

Azure/azure-sdk-for-python#1752

Added new receivers to action group document.

59af5b9

azuresdkci assigned fearthecowboy Dec 1, 2017

fearthecowboy assigned marstr and unassigned fearthecowboy Dec 4, 2017

marstr requested changes Dec 4, 2017

View reviewed changes

REDMOND\shriku added 2 commits December 4, 2017 13:51

fixed few comments.

8cd8b0c

Corrected the file syntactically using openapi-linter

283f3ad

marstr requested changes Dec 5, 2017

View reviewed changes

marstr requested review from simongdavies and ravbhatnagar December 5, 2017 22:12

REDMOND\shriku added 2 commits December 8, 2017 18:54

Add required patch operation for which it is flagging this documentation

68748c5

Fixed some liter format validation issues, added patch example

8e429b0

marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Dec 12, 2017

marstr previously requested changes Dec 13, 2017

View reviewed changes

REDMOND\shriku added 2 commits December 13, 2017 16:09

Fixed couple for validation errors

d00501b

Fixed new validation error for runbook receiver

0ef0531

ravbhatnagar reviewed Dec 14, 2017

View reviewed changes

fixed the description field for ITSM Ticket configuration field.

3ffbce8

marstr changed the base branch from current to master December 28, 2017 17:58

REDMOND\shriku added 7 commits January 3, 2018 09:12

Added new receivers to action group document.

519230d

fixed few comments.

4782243

Corrected the file syntactically using openapi-linter

dbd23df

Add required patch operation for which it is flagging this documentation

6563da7

Fixed some liter format validation issues, added patch example

db04cc2

Fixed couple for validation errors

dcc811e

Fixed new validation error for runbook receiver

e9e4271

fixed the description field for ITSM Ticket configuration field.

eada97a

marstr force-pushed the documentation branch from 3ffbce8 to eada97a Compare January 3, 2018 17:18

REDMOND\shriku added 2 commits January 3, 2018 10:10

merge from remote

c002bd9

Added example file for patch operation to new directory called stable.

83964be

marstr assigned veronicagg and unassigned marstr Jan 8, 2018

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jan 9, 2018

Merge branch 'master' into documentation

7a323fa

veronicagg removed the request for review from simongdavies January 9, 2018 22:25

veronicagg added the potential-sdk-breaking-change label Jan 9, 2018

veronicagg approved these changes Jan 9, 2018

View reviewed changes

veronicagg merged commit e446753 into Azure:master Jan 9, 2018

AutorestCI mentioned this pull request Jan 9, 2018

Automatic PR from RestAPI-PR2077 Azure/azure-sdk-for-ruby#1227

Closed

AutorestCI mentioned this pull request Jan 9, 2018

[MgmtMonitor] Added new receivers to action group document. Azure/azure-sdk-for-python#1752

Closed

Added new receivers to action group document. #2077

Added new receivers to action group document. #2077

Conversation

shriku11 commented Dec 1, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

fearthecowboy commented Dec 4, 2017

marstr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marstr 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

marstr commented Dec 5, 2017 • edited Loading

shriku11 commented Dec 8, 2017

marstr left a comment

Choose a reason for hiding this comment

ravbhatnagar 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

shriku11 commented Jan 2, 2018

marstr commented Jan 3, 2018 • edited Loading

azuresdkciprbot commented Jan 3, 2018

shriku11 commented Jan 3, 2018 via email

marstr commented Jan 5, 2018

marstr commented Jan 8, 2018

ravbhatnagar commented Jan 9, 2018

veronicagg commented Jan 9, 2018

azuresdkciprbot commented Jan 9, 2018

shriku11 commented Jan 9, 2018

veronicagg left a comment

Choose a reason for hiding this comment

AutorestCI commented Jan 9, 2018

AutorestCI commented Jan 9, 2018

shriku11 commented Dec 1, 2017 •

edited by azuresdkci

Loading

marstr commented Dec 5, 2017 •

edited

Loading

marstr commented Jan 3, 2018 •

edited

Loading