Skip to content

Comments

api spec for speech #5445

Closed
PanosPeriorellis wants to merge 9 commits intoAzure:masterfrom
PanosPeriorellis:master
Closed

api spec for speech #5445
PanosPeriorellis wants to merge 9 commits intoAzure:masterfrom
PanosPeriorellis:master

Conversation

@PanosPeriorellis
Copy link

@PanosPeriorellis PanosPeriorellis commented Mar 21, 2019

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.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@AutorestCI
Copy link

AutorestCI commented Mar 21, 2019

Automation for azure-sdk-for-python

Encountered an unknown error: (azure-sdk-for-python)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git checkout 01a7f19141d71d72231ca89e2135188ab2934b07
  stderr: 'fatal: reference is not a tree: 01a7f19141d71d72231ca89e2135188ab2934b07'

@AutorestCI
Copy link

AutorestCI commented Mar 21, 2019

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#5030

@AutorestCI
Copy link

AutorestCI commented Mar 21, 2019

Automation for azure-sdk-for-ruby

Encountered an unknown error: (azure-sdk-for-ruby)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git checkout 01a7f19141d71d72231ca89e2135188ab2934b07
  stderr: 'fatal: reference is not a tree: 01a7f19141d71d72231ca89e2135188ab2934b07'

@AutorestCI
Copy link

AutorestCI commented Mar 21, 2019

Automation for azure-sdk-for-java

Encountered an unknown error: (azure-sdk-for-java)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git checkout 01a7f19141d71d72231ca89e2135188ab2934b07
  stderr: 'fatal: reference is not a tree: 01a7f19141d71d72231ca89e2135188ab2934b07'

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Mar 21, 2019 via email

@dsgouda
Copy link
Contributor

dsgouda commented Mar 21, 2019

We have a separate process to onboard new RPs for the Net Sdk, please disregard Automation for azure-sdk-for-net related failures

@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Mar 21, 2019 via email

@erhopf
Copy link

erhopf commented Mar 21, 2019

@shahabhijeet Do you know why the sdk-for-net is failing?

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

  1. Given that this appears to be a completety new spec, has it been reviewed by the Azure API review board?
  2. Could you include someone else from your team to sing-off on the content of the PR?
  3. In order to generate SDKs we need readme.md configuration files, please take a look at https://github.com/Azure/azure-rest-api-specs/tree/cc3861000c4cfe6b02c3dcd84ccb875d0968f4b3/specification/cognitiveservices/data-plane/CustomVision/Training and include these in the PR.
  4. Are these APIs already deployed?

@veronicagg veronicagg added the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Mar 21, 2019
@erhopf
Copy link

erhopf commented Mar 21, 2019

@veronicagg - Thanks for the response.

  1. As this is a production service and the docs are already published, the API review was waived. We've been working with Mike Kinsman to get this published. Here's the request: https://ceapex.visualstudio.com/Onboarding/_workitems/edit/72351
  2. @PanosPeriorellis can review and sign-off on the content edits included in this PR.
  3. We'll review and update accordingly.
  4. Yes. https://westus.cris.ai/swagger/ui/index#/

@erhopf
Copy link

erhopf commented Mar 21, 2019

@veronicagg - Can you please link me to guidelines for creating the readme configuration files?

@veronicagg
Copy link
Contributor

veronicagg commented Mar 21, 2019

@erhopf general guidance is at https://github.com/Azure/autorest/blob/87a5600e4af7ea185a887c63ad3153f0936fb0fa/docs/user/configuration.md
but I recommend that you copy form something similar to you, feel free to take a look at the latest PR I merged for Custom Vision 3.0 #5164

@PanosPeriorellis
Copy link
Author

#sign-off

@nschonni
Copy link
Contributor

@PanosPeriorellis is there a reason for marking the typos as resolved without pushing anything? I know there is a bug with the GitHub preview on large files showing the wrong line on this view, but you can see the issues looking at the file view

-corrected levvel-> level and occured -> occurred
@AutorestCI
Copy link

AutorestCI commented Mar 22, 2019

Automation for azure-sdk-for-js

Encountered an unknown error: (azure-sdk-for-js)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git checkout 01a7f19141d71d72231ca89e2135188ab2934b07
  stderr: 'fatal: reference is not a tree: 01a7f19141d71d72231ca89e2135188ab2934b07'

@AutorestCI
Copy link

AutorestCI commented Mar 22, 2019

Automation for azure-sdk-for-go

Encountered an unknown error: (azure-sdk-for-go)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git checkout 01a7f19141d71d72231ca89e2135188ab2934b07
  stderr: 'fatal: reference is not a tree: 01a7f19141d71d72231ca89e2135188ab2934b07'

@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Mar 22, 2019 via email

-added all read me files required following the example of FACE (seemed closer than custom vision)
@PanosPeriorellis
Copy link
Author

I added all the readme files that are required following the example from FACE

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

@veronicagg
Copy link
Contributor

@shahabhijeet @dsgouda is the failure for .net generation due to being a new provider? should it be ignored?

@dsgouda
Copy link
Contributor

dsgouda commented Mar 22, 2019

@shahabhijeet @dsgouda is the failure for .net generation due to being a new provider? should it be ignored?

Yes, please ignore, we will on board them in the SDK repo separately

@veronicagg veronicagg removed the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Mar 22, 2019
@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Mar 25, 2019 via email

@erhopf
Copy link

erhopf commented Mar 25, 2019

@veronicagg - What are the next steps? Do we owe you anything?

@veronicagg
Copy link
Contributor

@PanosPeriorellis @erhopf yes, per my request of changes above, please review and correct CI errors for linter check https://github.com/Azure/azure-rest-api-specs/pull/5445/checks?check_run_id=82863674

- fixed some errors related to "format": "uri"
fixed anonymous type error
update 1 of 2 blokcs
@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Mar 26, 2019 via email

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

few comments inline

}
}
}
"$ref": "#/definitions/IReadOnlyDictionary`2"
Copy link
Contributor

@veronicagg veronicagg Mar 27, 2019

Choose a reason for hiding this comment

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

is the "`" an intentional character in this name? When generating SDK code, this name will turn into a Class name.

},
"definitions": {
"IReadOnlyDictionary`2": {
"title": "IReadOnlyDictionary`2",
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: please indent the properties

},
"definitions": {
"IReadOnlyDictionary`2": {
"title": "IReadOnlyDictionary`2",
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for Title here

"IReadOnlyDictionary`2": {
"title": "IReadOnlyDictionary`2",
"type": "object",
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add descriptions for these properties?

"type": "object",
"properties": {
"None": {
"none": {
Copy link
Contributor

Choose a reason for hiding this comment

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

does the casing update reflect the service? Overall, we need to make sure the swagger reflects the service, so if updates made to the swagger due to issues reported by tools do not reflect the service, and the service can't be changed for this api version, you can ask for exceptions for the issues.

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

More comments inline.

]
}
},
"401": {
Copy link
Contributor

Choose a reason for hiding this comment

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

please see https://github.com/Azure/azure-rest-api-specs/blob/da93a8f5a90e63c882d176021d6b935671faf0ba/documentation/creating-swagger.md#responses in particular https://github.com/Azure/azure-rest-api-specs/blob/da93a8f5a90e63c882d176021d6b935671faf0ba/documentation/creating-swagger.md#negative-responses
the recommendation is to have a "default" response for error responses which return an error model. If the spec describes each code, AutoRest for SDK code generation will have an "Object" return type for the operations, as "Error" and your actual return type have no other ancestor in common.

],
"parameters": [
{
"name": "id",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: you could abstract the parameter to the global parameter section, since you're referencing from multiple operations.

"Custom Speech accuracy tests:"
],
"summary": "Gets the list of accuracy tests for the authenticated subscription.",
"operationId": "GetAccuracyTests",
Copy link
Contributor

Choose a reason for hiding this comment

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

Operation ID should be of the form NOUN_VERB, the NOUN will become the "operation group" when generating SDK code, basically the name of the class in which all operations which shared it can be under. Ex. AccuracyTest_List

"Custom Speech accuracy tests:"
],
"summary": "Creates a new accuracy test.",
"operationId": "CreateAccuracyTest",
Copy link
Contributor

Choose a reason for hiding this comment

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

AccuracyTest_Create ?

"Custom Speech accuracy tests:"
],
"summary": "Gets the accuracy test identified by the given ID.",
"operationId": "GetAccuracyTest",
Copy link
Contributor

Choose a reason for hiding this comment

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

AccuracyTest_Get

"properties": {
"AcceptedLines": "11",
"RejectedLines": "2",
"ReportUri": "https://www.example.org/AcousticData/ed9b62f0-aa8e-419c-ab55-d7ca83c2f6b3?st=2018-02-09T18%3A07%3A00Z&se=2018-02-10T18%3A07%3A00Z&sp=r
Copy link
Contributor

Choose a reason for hiding this comment

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

"properties": {
"AcceptedLines": "11",
"RejectedLines": "2",
"ReportUri": "https://www.example.org/AcousticData/ed9b62f0-aa8e-419c-ab55-d7ca83c2f6b3?st=2018-02-09T18%3A07%3A00Z&se=2018-02-10T18%3A07%3A00Z&sp=r
Copy link
Contributor

Choose a reason for hiding this comment

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

long running? see my comment above referring to this

"properties": {
"AcceptedLines": "11",
"RejectedLines": "2",
"ReportUri": "https://www.example.org/AcousticData/ed9b62f0-aa8e-419c-ab55-d7ca83c2f6b3?st=2018-02-09T18%3A07%3A00Z&se=2018-02-10T18%3A07%3A00Z&sp=r
Copy link
Contributor

Choose a reason for hiding this comment

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

please review https://github.com/Azure/autorest/tree/master/docs/extensions#x-ms-enum Using this extension and choosing modelAsString: true, can avoid breaking changes in the future as more values get added. Please evaluate this for all enums used in the spec.

@veronicagg
Copy link
Contributor

@veronicagg
Copy link
Contributor

@PanosPeriorellis regarding your question on examples, examples inline should also work, and in that case you could request an exception for the warning https://github.com/Azure/adx-documentation-pr/wiki/Swagger-Validation-Errors-Suppression (make sure you're logged in to Github and have your account linked to Azure org to see this page).
We recommend our custom x-ms-examples extension, as it makes the spec more readable without the inline examples.
Regarding the examples showing up in documentation, @erhopf can you confirm this is the case?

@veronicagg
Copy link
Contributor

Additionally, you may want to take a look at the generated code from the SDKs (comments with generated PRs posted in the comments) to see if at least one the languages look like what you would expect. Thanks!

updated based on veronica's suggestions.
updated the response codes
Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments @PanosPeriorellis .
Regarding the suppression, since it needs to be fixed at some point, we won’t be suppressing (sorry about my previous comment). I will approve and merge with the error, once all the other comments are resolved.
Regarding my comment about the error codes for operations, it looks like you’d like to go ahead with what you have, is that correct?
I posted a couple of suggestions incline and here are some questions:
Please make sure to review warning and decide whether you want to fix them or not:
https://dev.azure.com/azure-sdk/public/_build/results?buildId=17447&view=logs&jobId=688669d0-441c-57c3-cf6d-f89a22ccfa5d&taskId=923b5a90-e462-5994-c166-86509c6bf5f2&lineStart=1111&lineEnd=1112&colStart=1&colEnd=1
I see errors reported from CI for:
Model validation for the examples inline the spec: https://dev.azure.com/azure-sdk/public/_build/results?buildId=17447&view=logs&jobId=8d2f9c49-cd83-5a9d-f3fe-2fd651afa742&taskId=43324f1f-6724-5d61-785f-f0ac9fd256a1&lineStart=63&lineEnd=64&colStart=1&colEnd=1

Once you confirm you’re done with changes and happy with the current state, I can approve and merge, please know that without addressing the error code responses comment, your SDKs will not have a good customer experience on return type for operations.
To request SDK releases please use https://portal.azure-devex-tools.com/app/tools/request-api-release. If you change how things look in the future, be aware that it can result in breaking changes.

],
"summary": "Gets the list of models for the authenticated subscription.",
"operationId": "GetModels",
"operationId": "Models_Get",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Model_List

],
"summary": "Gets a list of transcriptions for the authenticated subscription.",
"operationId": "GetTranscriptions",
"operationId": "Transcriptions_Get",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Transcription_List

"Custom Speech accuracy tests:"
],
"summary": "Gets the list of accuracy tests for the authenticated subscription.",
"operationId": "AccuracyTests_List",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update to AccuracyTest_List to make it inconsistent with the rest of the methods, like AccuracyTest_Create, since it's causing SDK generation issues in Java for example https://dev.azure.com/azure-sdk/public/_build/results?buildId=17447&view=logs&jobId=ef1a6f21-3eb6-5439-fa7a-dcd9cf589dd1&taskId=7d38bc47-3f36-578f-2b95-a19eb7179ec4&lineStart=155&lineEnd=156&colStart=1&colEnd=1

Copy link
Author

Choose a reason for hiding this comment

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

Btw, I will not expose this to customers just yet, or at least until we fix the error. I just want to complete this so we get on a roll, get to know your process and start making progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PanosPeriorellis The fact of merging it to master means it's exposed to customers. Why do you want to merge this PR to master in the current state?
If you're intending to auto-generate SDKs we need to make sure that what we merge, meets the quality bar for SDKs. Java SDK generation is currently failing due to the Model name inconsistencies and the way return codes are specified for negative responses are no good for SDK auto generation. I understand the APIs are in GA, but the changes requested affect SDK generation.

@veronicagg
Copy link
Contributor

@PanosPeriorellis I haven't heard back in more than 10 days on this PR, so I'll be closing it on Monday, and can be reopened when you get back to it. Thanks!

@PanosPeriorellis
Copy link
Author

PanosPeriorellis commented Apr 26, 2019 via email

@veronicagg
Copy link
Contributor

@PanosPeriorellis sounds good, thanks for following up. Based on the timeline, I'll close it and you can tag me back when you're back to this. Thanks!

@veronicagg veronicagg closed this Apr 26, 2019
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.

7 participants