[Merge-on-date: (TBA)] Add container log APIs to WebApps.json #1681

michimune · 2017-09-13T20:41:17Z

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-09-13T21:24:37Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+        "tags": [
+          "WebApps"
+        ],
+        "summary": "Gets the last lines of docker logs for the given site",


did you want to add an appropriate produces element? (see IANA Media Types )

"produces": [ "text/plain" ],

Please let me know if I'm missing something here but this endpoint does seem to work for me:

POST https://management.azure.com/subscriptions/ffa52f27-be12-4cad-b1ea-c2c241b6cceb/resourceGroups/linux-test-appgroup/providers/Microsoft.Web/sites/linux-test-app/containerlogs?api-version=2016-08-01 405 Method Not Allowed 749.00 ms { "error": { "Message": "The requested resource does not support http method 'POST'." } }

The deployment to production happened two weeks ago. I have confirmed it working with my testa pp. Can you please try again?

Seems like the endpoint is returning a base 64 encoded content - in this case, the return type shouldn't be file but byte. The file response type should be unencoded (gzipped okay).

Also, the server is actually returning me application/json for content type.

fearthecowboy · 2017-09-13T21:26:09Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+        ],
+        "summary": "Gets the last lines of docker logs for the given site",
+        "description": "Gets the last lines of docker logs for the given site",
+        "operationId": "WebApps_GetWebSiteContainerLogs",


And maybe here too?

"produces": [ "application/zip" ],

(and for the other APIs too...)

Same for this one - get application/json for content type, which probably requires a server-side fix and the response should be byte.

fearthecowboy

Looks pretty good to me; you might want to add produces on the APIs

Also, new APIs require signoff from ARM.

Adding @ravbhatnagar for his blessing.

ravbhatnagar · 2017-09-14T07:52:33Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/containerlogs/zip": {


missing segments. containerLogs is the resource type as per the signature. So you need a resourceTypename segment on which you will call zip action. A better way to model this API would be return a pointer to storage blob where the zip can be downloaded from. We generally avoid content other than application/json flowing through ARM.

Hmm, I see. Just to share the history. We initially used GET for this API but changed to POST based on suggestions from the CSM team (In order to return a byte stream). So zip was originally meant to be a resource type name.
Unfortunately returning a pointer to storage blob isn't our option because the zip is pointed to by a SCM URL where basic auth is used. We need to avoid returning a URL containing a clear password. Another possibility is that we store zip contents into storage blob which is specified by a command line parameter. However this could introduce more complexity to how the API is used.

We will revisit later for better modeling. For now, we would slightly change the signature by appending /download (i.e. ".../containerlogs/zip/download"). Does this make more sense?

ravbhatnagar · 2017-09-14T07:53:29Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

@@ -1913,6 +1913,84 @@
        }
      }
    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/containerlogs": {


May be rename the action to getContainerLogs?

We might want to add a new getContainerLogs later, but as of now we would like to keep the name.

azuresdkciprbot · 2017-09-19T02:05:41Z

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/web/resource-manager/readme.md
Before the PR: Warning(s): 1860 Error(s): 105
After the PR: Warning(s): 1870 Error(s): 105

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

fearthecowboy · 2017-09-19T15:06:53Z

When @ravbhatnagar gives his ascent to your feedback, I can merge this.

jianghaolu · 2017-09-20T18:45:31Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/containerlogs/zip/download": {
+      "post": {


Also this one

POST https://management.azure.com/subscriptions/ffa52f27-be12-4cad-b1ea-c2c241b6cceb/resourceGroups/linux-test-appgroup/providers/Microsoft.Web/sites/linux-test-app/containerlogs/zip/download?api-version=2016-08-01 404 Not Found The response was empty

salameer · 2017-09-25T19:29:21Z

@michimune and @ravbhatnagar

What are the update on this PR?

jianghaolu · 2017-10-16T22:33:44Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+        "tags": [
+          "WebApps"
+        ],
+        "summary": "Gets the last lines of docker logs for the given site",


Seems like the endpoint is returning a base 64 encoded content - in this case, the return type shouldn't be file but byte. The file response type should be unencoded (gzipped okay).

Also, the server is actually returning me application/json for content type.

jianghaolu · 2017-10-16T22:34:23Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

+        ],
+        "summary": "Gets the last lines of docker logs for the given site",
+        "description": "Gets the last lines of docker logs for the given site",
+        "operationId": "WebApps_GetWebSiteContainerLogs",


Same for this one - get application/json for content type, which probably requires a server-side fix and the response should be byte.

michimune · 2017-10-16T23:33:05Z

I can change the response schema type, but I have no idea how to specify the content type. I will need some investigation. Any advice would be helpful.

jianghaolu · 2017-10-17T00:06:36Z

@michimune It's a service issue - can you report to the service owner? The issue is these 2 APIs are sending non-JSON content with application/json content type header.

azuresdkciprbot · 2017-10-28T01:04:23Z

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

jianghaolu

The specs are approved. Waiting for the changes to be deployed to production.

ravbhatnagar · 2017-10-31T20:43:27Z

Looks good for ARM. Lets merge once the changes are deployed to prod.

fearthecowboy · 2017-12-08T17:23:04Z

@michimune -- When is the expected date this will deployed to production?

jianghaolu · 2017-12-08T19:35:03Z

@fearthecowboy I just verified this is live now.

fearthecowboy · 2017-12-08T19:36:22Z

Thanks @jianghaolu ... merged!

AutorestCI · 2017-12-08T19:40:35Z

Azure/azure-sdk-for-ruby#1173

AutorestCI · 2017-12-08T19:40:58Z

Azure/azure-sdk-for-python#1687

Add container log APIs to WebApps.json

68bba4f

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

azuresdkci assigned fearthecowboy Sep 13, 2017

fearthecowboy assigned ravbhatnagar Sep 13, 2017

fearthecowboy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 13, 2017

fearthecowboy reviewed Sep 13, 2017

View reviewed changes

fearthecowboy suggested changes Sep 13, 2017

View reviewed changes

ravbhatnagar suggested changes Sep 14, 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 14, 2017

fearthecowboy added the Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review label Sep 14, 2017

Update signatures of container logging APIs

fee6132

fearthecowboy approved these changes Sep 19, 2017

View reviewed changes

jianghaolu reviewed Sep 20, 2017

View reviewed changes

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

Azure deleted a comment from msftclas Sep 27, 2017

jianghaolu suggested changes Oct 16, 2017

View reviewed changes

Michimune Kohno and others added 2 commits October 27, 2017 14:26

Change schema types for container logging APIs

a194569

Resolving conflict

cc11211

Fix container log APIs

874a8a6

jianghaolu approved these changes Oct 30, 2017

View reviewed changes

ravbhatnagar approved these changes Oct 31, 2017

View reviewed changes

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ReadyForSDKReview labels Oct 31, 2017

fearthecowboy removed the Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review label Dec 8, 2017

fearthecowboy changed the title ~~Add container log APIs to WebApps.json~~ [Merge-on-date: (TBA)] Add container log APIs to WebApps.json Dec 8, 2017

fearthecowboy merged commit 58b5052 into Azure:current Dec 8, 2017

AutorestCI mentioned this pull request Dec 8, 2017

Automatic PR from RestAPI-PR1681 Azure/azure-sdk-for-ruby#1173

Closed

AutorestCI mentioned this pull request Dec 8, 2017

[WebApp] [Merge-on-date: (TBA)] Add container log APIs to WebApps.json Azure/azure-sdk-for-python#1687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merge-on-date: (TBA)] Add container log APIs to WebApps.json #1681

[Merge-on-date: (TBA)] Add container log APIs to WebApps.json #1681

michimune commented Sep 13, 2017 •

edited by azuresdkci

Loading

fearthecowboy Sep 13, 2017

michimune Sep 16, 2017

jianghaolu Sep 20, 2017 •

edited

Loading

michimune Oct 16, 2017

jianghaolu Oct 16, 2017

fearthecowboy Sep 13, 2017

michimune Sep 16, 2017

jianghaolu Oct 16, 2017

fearthecowboy left a comment

ravbhatnagar Sep 14, 2017

michimune Sep 18, 2017

ravbhatnagar Sep 14, 2017

michimune Oct 16, 2017

azuresdkciprbot commented Sep 19, 2017

fearthecowboy commented Sep 19, 2017

jianghaolu Sep 20, 2017

salameer commented Sep 25, 2017

jianghaolu Oct 16, 2017

jianghaolu Oct 16, 2017

michimune commented Oct 16, 2017

jianghaolu commented Oct 17, 2017

azuresdkciprbot commented Oct 28, 2017

jianghaolu left a comment

ravbhatnagar commented Oct 31, 2017

fearthecowboy commented Dec 8, 2017

jianghaolu commented Dec 8, 2017

fearthecowboy commented Dec 8, 2017

AutorestCI commented Dec 8, 2017

AutorestCI commented Dec 8, 2017

[Merge-on-date: (TBA)] Add container log APIs to WebApps.json #1681

[Merge-on-date: (TBA)] Add container log APIs to WebApps.json #1681

Conversation

michimune commented Sep 13, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianghaolu Sep 20, 2017 • edited Loading

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

fearthecowboy 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

azuresdkciprbot commented Sep 19, 2017

fearthecowboy commented Sep 19, 2017

Choose a reason for hiding this comment

salameer commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michimune commented Oct 16, 2017

jianghaolu commented Oct 17, 2017

azuresdkciprbot commented Oct 28, 2017

jianghaolu left a comment

Choose a reason for hiding this comment

ravbhatnagar commented Oct 31, 2017

fearthecowboy commented Dec 8, 2017

jianghaolu commented Dec 8, 2017

fearthecowboy commented Dec 8, 2017

AutorestCI commented Dec 8, 2017

AutorestCI commented Dec 8, 2017

michimune commented Sep 13, 2017 •

edited by azuresdkci

Loading

jianghaolu Sep 20, 2017 •

edited

Loading