-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Add a new field to SignalR Resource property #5661
Conversation
Automation for azure-sdk-for-pythonA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-jsA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-rubyA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-javaEncountered a Subprocess error: (azure-sdk-for-java)
Command: ['/usr/local/bin/autorest', '/tmp/tmpemwsrxce/rest/specification/signalr/resource-manager/readme.md', '--perform-load=false', '--swagger-to-sdk', '--output-artifact=configuration.json', '--input-file=foo', '--output-folder=/tmp/tmphook_9rm'] AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Failure:
Error: Unable to start AutoRest Core from /root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core
Error: Unable to start AutoRest Core from /root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core
at main (/opt/node_modules/autorest/dist/app.js:232:19)
at <anonymous>
/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist/app.js:33
autorest_core_1.Shutdown();
^
ReferenceError: autorest_core_1 is not defined
at process.on (/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist/app.js:33:5)
at emitOne (events.js:121:20)
at process.emit (events.js:211:7)
at process.emit (/node_modules/source-map-support/source-map-support.js:439:21)
fs.js:612
return binding.close(fd);
^
Error: EBADF: bad file descriptor, close
at Object.fs.closeSync (fs.js:612:18)
at StaticVolumeFile.shutdown (/opt/node_modules/autorest/dist/static-loader.js:352:10)
at StaticFilesystem.shutdown (/opt/node_modules/autorest/dist/static-loader.js:406:17)
at process.exit.n [as exit] (/opt/node_modules/autorest/dist/static-loader.js:169:11)
at printErrorAndExit (/node_modules/source-map-support/source-map-support.js:423:11)
at process.emit (/node_modules/source-map-support/source-map-support.js:435:16)
at process._fatalException (bootstrap_node.js:391:26) |
Can one of the admins verify this patch? |
REST Spec PR 'Azure/azure-rest-api-specs#5661' REST Spec PR Author 'juniwang' REST Spec PR Last commit
Automation for azure-sdk-for-netA PR has been created for you: |
@juniwang Please fix the merge conflicts |
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
stable version but additive changes, don't need ARM approval. |
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.
Will approve once merge conflicts are resolved.
|
@shahabhijeet @NelsonDaniel Please approve |
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.
@juniwang the diffs here are hard to read, can you pull latest changes from master and reapply your chanes
e8762b6
to
540a8bc
Compare
@dsgouda Thanks. Resetted using master branch and applid changes. |
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Outdated
Show resolved
Hide resolved
@dsgouda @shahabhijeet Could you have a review again? |
@shahabhijeet PTAL |
Hi @shahabhijeet, Could you please have another review? It seems that the PR is blocked by you |
REST Spec PR 'Azure/azure-rest-api-specs#5661' REST Spec PR Author 'juniwang' REST Spec PR Last commit
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.
Left a couple of comments
"additionalProperties": { | ||
"type": "string" | ||
}, | ||
"x-ms-client-flatten": false |
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.
nit: This doesn't need to be set explicitly
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.
Removed. This produces a warning:
WARNING (AvoidNestedProperties/R2001/SDKViolation): Consider using x-ms-client-flatten to provide a better end user experience
- file:///mnt/d/workspace/github/juniwang/azure-rest-api-specs/specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json:924:8 ($.definitions.SignalRFeature.properties.properties)
If it doens't matter, we may suppress the warning in future.
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, we discourage too many levels of nesting especially on input parameters hence the warning. Please mark x-ms-client-flatten: true
"type": "object", | ||
"properties": { | ||
"flag": { | ||
"description": "Name of the feature. 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 description is a bit confusing since there can only be a limited number of values. Type/kind of feature
makes more sense?
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
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 don't see any changes, not blocker though
@@ -319,6 +322,9 @@ | |||
} | |||
}, | |||
"x-ms-long-running-operation": true, | |||
"x-ms-long-running-operation-options": { | |||
"final-state-via": "azure-async-operation" |
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.
@juniwang any reason you are adding this to PUT operations? I was under the impression that this is only for POST.
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.
Telling from this doc: When x-ms-long-running-operation is specified, there should also be a x-ms-long-running-operation-options specified..
Will remove 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.
Removed
@@ -354,6 +360,9 @@ | |||
} | |||
}, | |||
"x-ms-long-running-operation": true, | |||
"x-ms-long-running-operation-options": { | |||
"final-state-via": "azure-async-operation" |
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.
@juniwang same as above, why specify for delete?
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.
Removed
REST Spec PR 'Azure/azure-rest-api-specs#5661' REST Spec PR Author 'juniwang' REST Spec PR Last commit
@shahabhijeet @dsgouda spec updated to rosolve comments |
@shahabhijeet PTAL |
@shahabhijeet any other comments? Or can someone review again on behalf of Abhijeet? |
Changes:
features
toSignalRResource
, which will used to query or update the FeatureFlags of SignalR resource. Corresponding examples updated.enum
property ofapi-version
. The extra enum will fail .NET SDK generationx-ms-long-running-operation-options
for long runnint operations.Contribution checklist: