APIs for WebJobs, Functions, SiteExtensions and Processes #1551
Conversation
* add deployments/log * add webjobs, continuouswebjobs, triggeredwebjobs * add siteextensions * add processes, instance/processes Diagnostics * add functions, functions/listsecrets, listsyncfunctiontriggerstatus
|
@EricSten-MSFT, |
There was a problem hiding this comment.
Please review duplicate operation ID errors: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/265311599#L653
veronicagg
added
the
WaitForARMFeedback
|
@ravbhatnagar new apis, please take a look, thanks! |
|
@veronicagg - I've made the requested changes. Can you unblock this PR? |
|
@EricSten-MSFT Semantic validation is still failing due to duplicate operatoin IDs: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/265357585#L656 (please check CI and under the "allowed failures section" mode=semantic, PR-only=true. The AutoRest Linter Azure bot (you can see a message pasted above) indicates there's an increase of errors and warnings introduced by this PR, could you please review the validation tool output? https://travis-ci.org/Azure/azure-rest-api-specs/jobs/265357584#L658 |
|
@EricSten-MSFT since there are new apis added in the PR, ARM team sign-off is required, @ravbhatnagar needs to review and approve. Thanks! |
|
@EricSten-MSFT are these API changes deployed to ARM already? |
| "operationId": "WebApps_ListContinuousWebJobs", | ||
| "produces": [ | ||
| "application/json", | ||
| "text/json" |
There was a problem hiding this comment.
only application/json is supported by arm, text/json should be removed if not supported. @ravbhatnagar
There was a problem hiding this comment.
It looks like all Antares (Azure Web Apps) APIs produce application/json, text/json, application/xml, and text/xml. I believe this is by design.
There was a problem hiding this comment.
Needs to be fixed all up. I will have that fixed as a separate PR across the board.
In reply to: 133797492 [](ancestors = 133797492)
| "tags": [ | ||
| "WebApps" | ||
| ], | ||
| "summary": "Get function information by its ID for a specific scaled-out instance in a web site.", |
There was a problem hiding this comment.
is this summary an description appropriate for this operation?
There was a problem hiding this comment.
Nope. Changing it to "Create function for web site, or a deployment slot."
| "description": "Get function information by its ID for a specific scaled-out instance in a web site.", | ||
| "operationId": "WebApps_CreateFunction", | ||
| "consumes": [ | ||
| "application/json", |
There was a problem hiding this comment.
only application/json is supported by ARM - @ravbhatnagar
There was a problem hiding this comment.
It looks like all Antares "Create" APIs consume application/json, text/json, application/x-www-form-urlencoded. I believe this is by design.
|
@veronicagg Update #2 should fix the duplicate operation IDs. |
|
Oh, and about the removed APIs (https://travis-ci.org/Azure/azure-rest-api-specs/jobs/265746191): Upon review, the ".../instance/{instanceId}..." variants of the deployments APIs should not have been exposed. Only the /slots/{slot} versions should be exposed. |
|
@veronicagg Regarding The AutoRest Linter Azure bot deltas: These all appear to be R3019 errors, which are on all of the existing APIs. I'm told it's an artifact of how the Azure Web Apps APIs were created, and is by design. |
|
Thanks @EricSten-MSFT for making the updates. |
|
@jianghaolu thanks for picking up this PR! |
| } | ||
| } | ||
| }, | ||
| "FunctionSecrets": { |
There was a problem hiding this comment.
Should this be a collection of secrets?
There was a problem hiding this comment.
No. It's just the two fields which are secret: key and triggerUrl.
There was a problem hiding this comment.
So a function can only have one secret?
There was a problem hiding this comment.
Yes. See Functions-API from the Kudu wiki
| "description": "Details of MSDeploy operation", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/MSDeploy" |
There was a problem hiding this comment.
MSDeploy still consists incorrect swagger - additional properties other than $ref are all ignored.
There was a problem hiding this comment.
are you talking about the "schema" block? If so, I don't understand your comment. The only thing in the "schema" is
"schema": { "$ref": "#/definitions/MSDeploy" }
There was a problem hiding this comment.
There was a problem hiding this comment.
Here's the source that's used to generate that Swagger:
(...) [RequestType(typeof(MSDeploy), Name = "MSDeploy", Description = "Details of MSDeploy operation", Required = true)] [ResponseType(typeof(MSDeployStatus))] [AzureOperationSettings(longRunning: true)] [HttpPut] public HttpResponseMessage CreateMSDeployOperation(string subscriptionId, string resourceGroupName, string name, string slot = null, string instanceId = null) (...)
Please tell me the magic annotation I need to put on this to make this problem go away. ;)
There was a problem hiding this comment.
@ Eric. I will take care of this one. The generator tool does not handle this very well today. I am working on a fix.
In reply to: 134351851 [](ancestors = 134351851)
| "$ref": "#/parameters/subscriptionIdParameter" | ||
| }, | ||
| { | ||
| "$ref": "#/parameters/apiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "201": { |
There was a problem hiding this comment.
Does 201 indicate a long running operation in progress?
There was a problem hiding this comment.
Please add x-ms-long-running-operation for this method.
| @@ -13936,6 +16885,10 @@ | |||
| } | |||
| } | |||
| }, | |||
| "Object": { | |||
There was a problem hiding this comment.
ContinuousWebJob and TriggeredWebJob both have a field, "settings", for which we can take arbitrary JSON. I'm not to sure of the details, but I believe this element is a result of how the tooling spews Swagger.
There was a problem hiding this comment.
Can you just use "type": object for those?
There was a problem hiding this comment.
Let me work with @naveedaz to see if I can author this so the tooling which spews the Swagger doesn't emit this.
There was a problem hiding this comment.
For now, I've hand-edited the WebApps.json swagger to remove the "Object" class and all references to it. It should be in the most recent push.
| "Object": { | ||
| "type": "object", | ||
| "properties": {} | ||
| }, | ||
| "Operation": { |
There was a problem hiding this comment.
"Operation" is not part of my change.
| "description": "Base address. Used as module identifier in ARM resource URI.", | ||
| "type": "string" | ||
| }, | ||
| "fileName": { |
There was a problem hiding this comment.
Please provide more useful descriptions for these properties.
There was a problem hiding this comment.
Is your comment about "Base address"? If so, then I don't know how to clarify it further: It's the module's base address (like what you'd find in windbg, cdb, etc.).
There was a problem hiding this comment.
It's about all the properties in this type. The descriptions are simply a rewording of the property name. What is a "file" in this type? What is the URI pointing to?
There was a problem hiding this comment.
This is what I had to go on: https://github.com/projectkudu/kudu/blob/5e26bd0d5d81a6aa92303cea0370a7e491e63923/Kudu.Contracts/Diagnostics/ProcessModuleInfo.cs
As you can see, there are zero comments on any of these fields. So, I simply put in the best comment available. Here's the wiki page describing the functionality:
https://github.com/projectkudu/kudu/wiki/Process-Threads-list-and-minidump-gcdump-diagsession#summary
Again, no description of what the fields. I presume they come from System.Diagnostics.ProcessModule
There was a problem hiding this comment.
@navyahmed @EricSten-MSFT - folks these APIs that are being added have a large number of issues. I started reviewing and realized whether you be able to address the feedback or not? If you cant address the feedback as these have existed on the service side for quite some time then I wont spend my time reviewing it and providing feedback. In this case, we will try and get the Web swagger issues fixed by giving you folks sufficient amount of time. Please let me know.
| "properties": { | ||
| "description": "ContinuousWebJob resource specific properties", | ||
| "properties": { | ||
| "status": { |
There was a problem hiding this comment.
Decorate with something akin to:
[PropertySettings(AllowedStringValues = new[] { "Value1", "Value2", "Value3" }, ModelValuesAsString = true, AllowedStringValueCollectionName = "WebJobStatus")]
In reply to: 134406282 [](ancestors = 134406282)
There was a problem hiding this comment.
Enum added. Should be in Update #4
| "description": "Log URL.", | ||
| "type": "string" | ||
| }, | ||
| "name": { |
There was a problem hiding this comment.
Why is this property needed again? Name of the resource comes from the URL and is also present in the ARM top level name property
There was a problem hiding this comment.
Log URL? It's the URL of the log file for the function (history of times the function was called, and any diagnostic information during each of the runs).
Name? It's the function name, e.g. "MyFunction" or "DoSomething".
There was a problem hiding this comment.
Artifact of the ARM free KUDU world.
@ Eric, annotate the property with [ApiExplorerSettings(IgnoreApi = true)] in model and regenerate the swagger.
In reply to: 134406400 [](ancestors = 134406400)
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm looping back with the owning devs to figure out why this is here. I don't think I can ignore the property, since Kudu may be depending upon it.
There was a problem hiding this comment.
A collection of ContinuousWebJobs is returned from ListContinuousWebJobs. The "name" field is needed to differentiate between the web jobs in the collection.
Same goes for TriggeredWebJobs and ListTriggeredWebJobs.
There was a problem hiding this comment.
Confirmed with owning devs: the name field is read-only and not used when creating.
There was a problem hiding this comment.
fix should be in Update #5
| "description": "Extra Info URL.", | ||
| "type": "string" | ||
| }, | ||
| "jobType": { |
There was a problem hiding this comment.
@ Eric, Is there a finite set of known valid values for this type? If so, please decorate with something like.
[PropertySettings(AllowedStringValues = new[] { "Value1", "Value2", "Value3" }, ModelValuesAsString = true, AllowedStringValueCollectionName = "WebJobType")]
In reply to: 134406455 [](ancestors = 134406455)
| "description": "Error information.", | ||
| "type": "string" | ||
| }, | ||
| "usingSdk": { |
There was a problem hiding this comment.
what does this property used for? Not very intuitive. May be make it an enum with a better property name. But this would mean service side change on your side.
There was a problem hiding this comment.
These are all KUDU APIs that are also being exposed in swagger to be compliant wit fundamentals. Changes to them would require revving KUDU. We should track these as issues, but for the scope of this PR we cannot update KUDU.
In reply to: 134406692 [](ancestors = 134406692)
| "properties": { | ||
| "description": "FunctionEnvelope resource specific properties", | ||
| "properties": { | ||
| "name": { |
There was a problem hiding this comment.
Not needed. Please remove
There was a problem hiding this comment.
@ Eric, annotate the property with [ApiExplorerSettings(IgnoreApi = true)] in model and regenerate the swagger.
In reply to: 134406880 [](ancestors = 134406880)
There was a problem hiding this comment.
Which property? If I'm reading Functions-API correctly, I think all of the properties are needed in FunctionEnvelope.
There was a problem hiding this comment.
The reason modifiable names are not allowed is that in this operation: https://github.com/EricSten-MSFT/azure-rest-api-specs/blob/449478bf179eae0001b9fd463355dd517e74fb6a/specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json#L2983 you could provide a different name here in the envelope from the one on the URL.
There was a problem hiding this comment.
A collection of FunctionEnvelope is returned in a response to ListFunctions operation. In that case, the name is required to differentiate between functions.
There was a problem hiding this comment.
In ARM we use a read-only name for that purpose.
There was a problem hiding this comment.
Update #5 should have the fix.
| "description": "Function name.", | ||
| "type": "string" | ||
| }, | ||
| "functionAppId": { |
There was a problem hiding this comment.
@ Eric if this is indeed readonly you can annotate it with [PropertySettings(Accessibility = PropertyAccessibility.ReadOnly)] in the model and regenerate the swagger.
In reply to: 134406926 [](ancestors = 134406926)
There was a problem hiding this comment.
Function name isn't read-only in the Kudu sources.
There was a problem hiding this comment.
I think @ravbhatnagar is talking about the function app id.
There was a problem hiding this comment.
The Kudu sources don't say that any of the FunctionEnvelope properties are read-only. The FunctionEnvelope is passed to the CreateFunction API. I don't know if the Kudu code expects the functionAppId to be set or not during the CreateFunction.
There was a problem hiding this comment.
If setting it throws an error - then it should be read only. If setting it does not throw an error - then if I set it with a random string, what does server return? If the server returns me the same random data, then the function id is wrong; if server returns me the correct function id, then it's not idempotent.
There was a problem hiding this comment.
Confirmed with owning devs: these fields (name, functionAppId) are read-only and not used when creating. The are used when returning a collection of FunctionEnvelopes.
| "type": "string" | ||
| } | ||
| }, | ||
| "testData": { |
There was a problem hiding this comment.
what needs to be provided as test data? better description will be helpful.
There was a problem hiding this comment.
@davidebbo Do you have a better description of "testData"? The Kudu wiki for functions doesn't really describe it.
There was a problem hiding this comment.
Checked with owning dev; updated the description accordingly. Fix should be in Update #5.
| "$ref": "#/definitions/Object", | ||
| "description": "Config information." | ||
| }, | ||
| "files": { |
There was a problem hiding this comment.
How are these files passed? Is this a pointer to a stroage location where the file resides?
There was a problem hiding this comment.
For "config": from the Kudu wiki, it looks like they're passed in-line in the JSON passed to PUT.
For "files", it's even scarier: looks like a dictionary of filename : file-contents elements. e.g.:
"files": {
"foo.bat" : "@echo off \r\necho I am a batch file!\r\necho Done!\r\n"
}
| @@ -9675,28 +10562,23 @@ | |||
| "200": { | |||
| "description": "OK", | |||
| "schema": { | |||
| "$ref": "#/definitions/PremierAddOn" | |||
| "$ref": "#/definitions/FunctionSecrets" | |||
There was a problem hiding this comment.
Isnt this a breaking change where the API signature and the response is changing? GET becomes a POST /listsyncfunctiontriggerstatus
There was a problem hiding this comment.
It is a completely different endpoint. The sitecloneability API is deprecated. The diff just looks odd here.
In reply to: 134407904 [](ancestors = 134407904)
|
Ping! I believe I've addressed all the items in the code review. What is the next step here? |
|
@EricSten-MSFT Thanks for addressing the feedbacks - what about the annotations Navy requested you to add in your RP? Thanks. |
|
@jianghaolu Yes, I've added the annotations. The current PR version has the right types. |
|
@EricSten-MSFT I left a few more comments where you might have overlooked. I understand the gap between Kudu and ARM and the complexities within - but if we can bridge it then the developers don't have to. Thanks! |
|
@ravbhatnagar @veronicagg All my concerns have been addressed. Is there anything you'd like to add before I merge this? |
|
Thank you! Let's put this puppy to bed...please merge! |
|
@jianghaolu I've dismissed my review, since I transferred this one to you, thanks for picking it up! |
|
Just checking: Will this be merged in soon? I have a PM that needs to report that APIs that were Red (undoc'd) are now Green (doc'd). |
Hand-merge WebApps.json since merge tools freaked out.
|
Looks like pull request 1566 went in, and it required some rather tricky hand-merging. |
|
@EricSten-MSFT You guys generate out the swagger spec right? |
|
@jianghaolu normally, yes. But for the sake of expediency, it was just easier to edit the WebApps.json directly. |
|
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: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
|
@jianghaolu And now it appears the hand-merge has broken things. Hold while I fix things. |
|
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: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
|
@jianghaolu Okay, fixed merge is in. It's what we generate. Unfortunately, there's a ton of documentation fixes (adding '.', etc.) that makes the last commit look a little noisy. |
|
@EricSten-MSFT LGTM. Merging. |
|
No modification for AutorestCI/azure-sdk-for-node |
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
api-versionin the path should match theapi-versionin the spec).Quality of Swagger