Release ServiceFabric 2017-07-01-preview swagger specification #1717

juhacket · 2017-09-21T21:35:29Z

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.

msftclas · 2017-09-21T21:35:34Z

@juhacket,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

sergey-shandar

@juhacket please, make changes in readme.md

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.

…specs into current

sergey-shandar · 2017-09-22T17:11:33Z

specification/servicefabric/resource-manager/readme.md

 ```


-### Tag: package-2016-09
+### Tag: package-2017-07


Thank you for adding a readme file. Please, add a new tag and keep package-2016-09 untouched. This way, we can generate SDK for different versions on demand and keep track of how we did it before.

@juhacket thank you, I will keep posting mini-review to fix CI until ARM approval. After that, I will send full review.

sergey-shandar · 2017-09-22T17:13:34Z

@ravbhatnagar it's a new API version for Service Fabric.

…specs into current

azuresdkciprbot · 2017-09-22T18:16:55Z

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

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

sergey-shandar · 2017-09-22T18:48:56Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+    },
+    "ServicePlacementPolicyType": {
+      "type": "string",
+      "description": "The type of placement policy for a service fabric service. Following are the possible values.\n\n  - Invalid - Indicates the type of the placement policy is invalid. All Service Fabric enumerations have the invalid type. The value is zero.\n  - InvalidDomain - Indicates that th


use true instead of "true" for Booleans. Search for "true" and "false" in the document and replace them.

We had this document validated by the ARM team already. Does it need to be reviewed again?

We are trying to publish it for Ignite.

@juhacket do you have an email/link to the validation? Another option (to make it faster) is to contact @ravbhatnagar directly so he would label the PR as ARM signed off.

ravbhatnagar

@juhacket - Some comments for you. Some of it minor and some that may need to be revisited (specially the one where you are referencing type and version from Application resource). I know this would already have been implemented in service. Can we get an ACK from you that you can take a look at addressing the feedback in the next APi-version if it seems like the right design?

ravbhatnagar · 2017-09-22T22:22:02Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+      "description": "If true, then processes are forcefully restarted during upgrade even when the code version has not changed (the upgrade only changes configuration or data).",
+      "default": false
+    },
+    "HealthCheckRetryTimeout": {


ISO 8601 standard. Format should be marked as date-time.

ravbhatnagar · 2017-09-22T22:22:08Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+    },
+    "HealthCheckStableDuration": {
+      "type": "string",
+      "description": "The amount of time that the application or cluster must remain healthy before the upgrade proceeds to the next upgrade domain. It is first interpreted as a string representing an ISO 8601 duration. If that fails, then it is interpreted as a number representing the total number of milliseconds.",


ISO 8601 standard. Format should be marked as date-time.

ravbhatnagar · 2017-09-22T22:22:22Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+      "description": "The amount of time that the application or cluster must remain healthy before the upgrade proceeds to the next upgrade domain. It is first interpreted as a string representing an ISO 8601 duration. If that fails, then it is interpreted as a number representing the total number of milliseconds.",
+      "default": "PT0H2M0S"
+    },
+    "HealthCheckWaitDuration": {


ISO 8601 standard.

ravbhatnagar · 2017-09-22T22:24:29Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+          }
+        },
+        "x-ms-long-running-operation": "true",
+        "responses": {


This is existing API i think, but 201 + provisioningState + AzureAsyncOperation header is recommended for PUT LRO instead of 202.

Cluster doesn't support async operations yet. This will be in another API version.

Lets mark "x-ms-long-running-operation": "true" as false then. It is currently set to true. And we can also remove the 202 from the response.

ravbhatnagar · 2017-09-22T22:30:21Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+    },
+    "ServicePlacementPolicyType": {
+      "type": "string",
+      "description": "The type of placement policy for a service fabric service. Following are the possible values.\n\n  - Invalid - Indicates the type of the placement policy is invalid. All Service Fabric enumerations have the invalid type. The value is zero.\n  - InvalidDomain - Indicates that th


It looks like all these state transitions happen during cluster provisioning. These values could be/should be merged with provisioningState. Existing API

ravbhatnagar · 2017-09-22T22:34:26Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/providers/Microsoft.ServiceFabric/locations/{location}/environments/{environment}/clusterVersions/{clusterVersion}": {


How does customer discover the list of environments?

environments is a property on the response of clusterVersions. This could be supported using a query parameter instead of modeling it as a resource. So to get cluster versions for a specific environment, customer will use this API -
/subscriptions/{subscriptionId}/providers/Microsoft.ServiceFabric/locations/{location}/clusterVersions and then do a $filter.

ravbhatnagar · 2017-09-22T22:38:04Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+          }
+        }
+      },
+      "patch": {


What is updatable through this patch? There is nothing in the properties bag.

Nothing since we changed this to a proxy resource.

Should we remove this API then?

ravbhatnagar · 2017-09-22T22:39:25Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ServiceFabric/clusters/{clusterName}/applicationTypes/{applicationTypeName}": {


Isnt applicationTypes a proxy resource? Proxy resources dont have a location, but in the response of this API, i see you have a location. Is it a tracked resource?

We were planning to make it a tracked resource, but decided against it. We left this as is in case we changed our mind.

ravbhatnagar · 2017-09-22T22:49:10Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+    },
+    "ServicePlacementPolicyType": {
+      "type": "string",
+      "description": "The type of placement policy for a service fabric service. Following are the possible values.\n\n  - Invalid - Indicates the type of the placement policy is invalid. All Service Fabric enumerations have the invalid type. The value is zero.\n  - InvalidDomain - Indicates that th


In application resource, you are refering the applicationTyperesource. It should be done using the arm resource id of the applicationTypeResource.

ravbhatnagar · 2017-09-22T22:49:44Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

+    },
+    "ServicePlacementPolicyType": {
+      "type": "string",
+      "description": "The type of placement policy for a service fabric service. Following are the possible values.\n\n  - Invalid - Indicates the type of the placement policy is invalid. All Service Fabric enumerations have the invalid type. The value is zero.\n  - InvalidDomain - Indicates that th


versions resource is being referenced here. It should be done using the arm resource id.

ravbhatnagar · 2017-09-22T23:30:25Z

@sergey-shandar - lets merge this. As you might have seen on the email, we are conditionally merging this as this is an Ignite deliverable but the team will address the feedback post Ignite. And also this is preview Api-version.
@salameer -another conditionally merged PR.

juhacket · 2017-09-26T17:02:37Z

@sergey-shandar Can you commit this change? It looks like all requirements have been met.

sergey-shandar · 2017-09-26T17:53:16Z

@juhacket @ravbhatnagar we need to fix at least errors which prevents us to generate Node.js SDK. In particular, convert string to bool which I mentioned before.

…specs into current

azuresdkciprbot · 2017-09-26T19:13:37Z

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

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

sergey-shandar

There are still some issues but we decided to merged this with the condition that the issues will be fixed ASAP.

AutorestCI · 2017-09-26T20:06:39Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-09-26T20:06:51Z

Azure/azure-sdk-for-python#1483

AutorestCI · 2017-09-26T20:07:39Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-09-26T20:35:19Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-09-26T20:35:41Z

No modification for AutorestCI/azure-sdk-for-node

lmazuel

Breaking change

lmazuel · 2017-09-26T21:32:40Z

specification/servicefabric/resource-manager/readme.md

@@ -25,8 +25,10 @@ To see additional help and options, run:
 These are the global settings for the ServiceFabricClient API.

 ``` yaml
+title: ServiceFabricClient


@juhacket @sergey-shandar
This is a breaking change and redefine the Swagger that correctly says ServiceFabricManagementClient

@juhacket could you send a PR to fix this problem ASAP. @lmazuel I missed this line which effects all versions of API.

amarzavery · 2017-09-28T01:56:02Z

@sergey-shandar @juhacket - This PR has issues.
The global parameters should have the "x-ms-parameter-location": "method" extension if they do not intend to be properties on the client.

The constructor signature of the client changed from
constructor(credentials, subscriptionId, baseUri, options)

to

constructor(credentials, applicationName, applicationTypeName, serviceName, subscriptionId, version, cluster, clusterUpdateParameters, baseUri, options)
which is a breaking and unnecessary change.
/cc @salameer
Azure/azure-openapi-validator#84

Release ServiceFabric 2017-07-01-preview swagger specification

40d7e2c

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.

msftclas added the cla-not-required label Sep 21, 2017

azuresdkci assigned sergey-shandar Sep 21, 2017

sergey-shandar suggested changes Sep 21, 2017

View reviewed changes

juhacket added 2 commits September 21, 2017 17:23

Release ServiceFabric 2017-07-01-preview swagger specification

d518884

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.

Merge branch 'current' of https://github.com/juhacket/azure-rest-api-…

c15cd0c

…specs into current

sergey-shandar suggested changes Sep 22, 2017

View reviewed changes

sergey-shandar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 22, 2017

juhacket added 2 commits September 22, 2017 11:03

Merge branch 'current' of https://github.com/juhacket/azure-rest-api-…

fac2ce8

…specs into current

Merge branch 'current' of https://github.com/juhacket/azure-rest-api-…

bc404dd

…specs into current

sergey-shandar suggested changes Sep 22, 2017

View reviewed changes

ravbhatnagar reviewed Sep 22, 2017

View reviewed changes

ravbhatnagar added ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 22, 2017

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review Conditionally-Merged labels Sep 22, 2017

msftgits removed the cla-not-required label Sep 26, 2017

juhacket added 2 commits September 26, 2017 12:09

Merge branch 'current' of https://github.com/juhacket/azure-rest-api-…

d54077b

…specs into current

Merge branch 'current' of https://github.com/juhacket/azure-rest-api-…

50a89de

…specs into current

sergey-shandar approved these changes Sep 26, 2017

View reviewed changes

sergey-shandar merged commit bc175db into Azure:current Sep 26, 2017

AutorestCI mentioned this pull request Sep 26, 2017

Automatic PR from RestAPI-PR1717 Azure/azure-sdk-for-python#1483

Closed

lmazuel reviewed Sep 26, 2017

View reviewed changes

amarzavery mentioned this pull request Sep 28, 2017

Fixing global parameters for siterecovery and resourcehealth #1753

Closed

Release ServiceFabric 2017-07-01-preview swagger specification #1717

Release ServiceFabric 2017-07-01-preview swagger specification #1717

Conversation

juhacket commented Sep 21, 2017 • edited by azuresdkci Loading

msftclas commented Sep 21, 2017

sergey-shandar 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

sergey-shandar commented Sep 22, 2017

azuresdkciprbot commented Sep 22, 2017

sergey-shandar Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergey-shandar Sep 22, 2017 • edited Loading

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

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

ravbhatnagar commented Sep 22, 2017

juhacket commented Sep 26, 2017

sergey-shandar commented Sep 26, 2017 • edited Loading

azuresdkciprbot commented Sep 26, 2017

sergey-shandar left a comment

Choose a reason for hiding this comment

AutorestCI commented Sep 26, 2017

AutorestCI commented Sep 26, 2017

AutorestCI commented Sep 26, 2017

AutorestCI commented Sep 26, 2017

AutorestCI commented Sep 26, 2017

lmazuel 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

amarzavery commented Sep 28, 2017

juhacket commented Sep 21, 2017 •

edited by azuresdkci

Loading

sergey-shandar Sep 22, 2017 •

edited

Loading

sergey-shandar Sep 22, 2017 •

edited

Loading

sergey-shandar commented Sep 26, 2017 •

edited

Loading