Skip to content
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

Adding azure accounts APIs #4524

Merged
merged 22 commits into from
Dec 13, 2018

Conversation

omarelhariry
Copy link
Contributor

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

@msftclas
Copy link

msftclas commented Nov 20, 2018

CLA assistant check
All CLA requirements met.

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-js

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-js#829

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3362

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#3700

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#4448

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@yangyuan
Copy link
Member

Hi @omarelhariry,

Have you reviewed changes with Azure API Review Board?
Feel free to contact me alias: yuanyang for further information.

@sarangan12
Copy link
Member

Please check the output of https://travis-ci.org/Azure/azure-rest-api-specs/jobs/460227670 and fix the errors

@omarelhariry
Copy link
Contributor Author

@sarangan12 Fixed, please check now.

@yangyuan
Copy link
Member

Hi @omarelhariry, any update from API review board?

@omarelhariry
Copy link
Contributor Author

I believe @sarangan12 review is sufficient for that as per their response.

@yangyuan
Copy link
Member

Hi @omarelhariry,
API Review Board will review the API design which won't be fully reviewed in here.
For example they will probably suggest to use lowerCamelCase in actions like azureaccounts.

@Azure Azure deleted a comment from openapi-portal-comment bot Nov 30, 2018
}
}
},
"PackageBinaryStreamObject": {
Copy link
Contributor

@AmirGeorge AmirGeorge Dec 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangyuan can you advise on what needs to be done here? The API returns a byte array (binary octet stream) in the response. How could we document its response here?

For context, we do not want to give the users visibility on how to deserialize the object. We just instruct our users to call that API to export the model and import it back in a different place. So we want to expose the minimum amount of details about the response format.

We tried have this as type array, type string & type object but the three fail giving the following error:

FATAL: System.Collections.Generic.KeyNotFoundException: Please specify a field with type of System.Byte[] to deserialize the file contents to
   at AutoRest.Modeler.OperationBuilder.VerifyFirstPropertyIsByteArray(CompositeType serviceType) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\OperationBuilder.cs:line 441
   at AutoRest.Modeler.OperationBuilder.TryBuildStreamResponse(HttpStatusCode responseStatusCode, OperationResponse response, Method method, List`1 types, IModelType headerType) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\OperationBuilder.cs:line 422
   at AutoRest.Modeler.OperationBuilder.BuildResponses(Method method, CompositeType headerType) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\OperationBuilder.cs:line 336
   at AutoRest.Modeler.OperationBuilder.BuildMethod(HttpMethod httpMethod, String url, String methodName, String methodGroup) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\OperationBuilder.cs:line 195
   at AutoRest.Modeler.SwaggerModeler.BuildMethod(HttpMethod httpMethod, String url, String name, Operation operation) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SwaggerModeler.cs:line 366
   at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SwaggerModeler.cs:line 105
   at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\Program.cs:line 60
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at NewPlugin.<Process>d__15.MoveNext()
FATAL: nodejs/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
  Error: Plugin imodeler1 reported failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test but I think it should be "format": "binary":

"PackageBinaryStreamObject": {
  "description": "The binary stream of the application package.",
  "type": "string",
  "format": "binary"
}

You should also add "content": "application/octet-stream" in response.

About "we do not want to give the users visibility on how to deserialize the object."
Why not just exclude this from swagger?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangyuan format binary is also not working, giving same error.

On adding content type, I already have:

"produces": [
          "application/octet-stream"
        ]

This is what we use in all our APIs to specify content type, is this what you mean? If there is another place to add "content": "application/octet-stream", please advise where.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to answer your suggestion about excluding from swagger, we want the API to be visible for our users to call it but we do not want them to parse the response. Just call this API to export the app and import it elsewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, format binary is pretty common, surprisingly it's not supported.

But if that's the case, please refer to https://github.com/Azure/azure-rest-api-specs/blob/master/specification/cognitiveservices/data-plane/ComputerVision/stable/v1.0/ComputerVision.json
and use "type": "file"

For octet-stream, produces should be good enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangyuan a gentle reminder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give me the exact command which causes the given error.
I started to think this might be other issues.

I checked travis log, didn't see this error.

Copy link
Contributor

@AmirGeorge AmirGeorge Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not running any manual commands, I see the error in the automated PR build checks:
https://github.com/Azure/azure-rest-api-specs/pull/4524/checks?check_run_id=37479764

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled your branch, and run
autorest specification/cognitiveservices/data-plane/LUIS/Authoring/ --go
And I can see the exact same error.

Then I modify the "responses" of "/package/{appId}/versions/{versionId}/gzip" "/package/{appId}/slot/{slotName}/gzip" into below

        "responses": {
          "200": {
            "description": "The GZip binary stream of the published application package.",
            "schema": {
              "type": "file"
            }
          }
        }

And the problem gone.
Which means "type": "file" is valid, and I suspect "format": "binary" can work too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debugged a little bit, this is the solution.

        "produces": [
          "application/octet-stream",
          "application/json"
        ],

The problem is you define the response can only be application/octet-stream, so autorest try to assert that all response should be binary format and gives error. :)

@sarangan12 sarangan12 removed their assignment Dec 3, 2018
@sarangan12 sarangan12 removed their request for review December 3, 2018 21:46
"$ref": "#/definitions/ProductionOrStagingEndpointInfo"
}
},
"503": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error response modelling is not supported in autorest as of now.

"$ref": "#/definitions/OperationStatus"
}
},
"429": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error responses are not supported by autorest as of now.

Add the error response to the default section. This applies all through this file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praries880 Can you explain more as the SDKs were generated successfully and the error responses were included in the SDKs with no problem like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this? @praries880

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praries880 any updates ? this is blocking our release before the holidays, appreciate if we can close on that before end of week

"$ref": "#/definitions/ProductionOrStagingEndpointInfo"
}
},
"503": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't document 5xx responses for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this one is thrown on purpose, it is not an arbitrary exception. Should it be removed too?

@praries880 praries880 merged commit 7d344e2 into Azure:master Dec 13, 2018
TalluriAnusha pushed a commit to AsrOneSdk/azure-rest-api-specs that referenced this pull request Feb 6, 2019
* Adding azure accounts APIs

* Adding Publishing To Regions Status

* fixes

* fixing build errors

* Fixing OAV errors

* Add packaging APIs

* fixing build errors

* apply yangyuan suggestion

* adding response content for gzip apis

* Revert "adding response content for gzip apis"

This reverts commit 3495ee0.

* PackageBinaryStreamObject type as file

* remove PackageBinaryStreamObject and directly add file type in response

* specify body format in packaging response examples

* adding content type in packaging response examples parameters

* maybe a casing issue?

* trying to remove the binary body from response parameters

* Add application-json as a 2nd prdocues format for packaging APIs, to describe the error responses

* fix

* Updating operation ids

* fix oav and remove error responses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.