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

[LogicApps] New Api version #6601

Merged
merged 43 commits into from
Sep 20, 2019
Merged

Conversation

visharm
Copy link
Contributor

@visharm visharm commented Jul 11, 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.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 11, 2019

In Testing, Please Ignore

[Logs] (Generated from 56286b2, Iteration 37)

Failed .NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Failed Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Failed logic/resource-manager/v2016_06_01 [Logs]
  • Failed logic/resource-manager/v2018_07_01_preview [Logs]
  • Failed logic/resource-manager/v2019_05_01 [Logs]
Failed Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Failed JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Succeeded Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]

@AutorestCI
Copy link

AutorestCI commented Jul 11, 2019

Automation for azure-sdk-for-python

A 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:
Azure/azure-sdk-for-python#7293

@AutorestCI
Copy link

AutorestCI commented Jul 11, 2019

Automation for azure-sdk-for-java

A 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:
Azure/azure-sdk-for-java#4006

@AutorestCI
Copy link

AutorestCI commented Jul 11, 2019

Automation for azure-sdk-for-go

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

@azuresdkci azuresdkci requested a review from praries880 July 11, 2019 22:07
@visharm visharm changed the title Visharm update swagger [LogicApps] New Api version Jul 11, 2019
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@praries880 praries880 added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jul 12, 2019
@visharm
Copy link
Contributor Author

visharm commented Jul 12, 2019 via email

Copy link
Contributor

@ryansbenson ryansbenson left a comment

Choose a reason for hiding this comment

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

Just some minor things that didn't come over from the last api version.

},
"parameters": [
{
"$ref": "#/parameters/subscription
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed from the previous api version and should be added back:

"default": {
"description": "Logic error response describing why the operation failed.",
"schema": {
"$ref": "#/definitions/ErrorResponse"
}
}

},
"parameters": [
{
"$ref": "#/parameters/subscription
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed from the previous api version and should be added back:

"default": {
"description": "Logic error response describing why the operation failed.",
"schema": {
"$ref": "#/definitions/ErrorResponse"
}
}

},
"parameters": [
{
"$ref": "#/parameters/subscription
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed from the previous api version and should be added back:

"default": {
"description": "Logic error response describing why the operation failed.",
"schema": {
"$ref": "#/definitions/ErrorResponse"
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just compared the swagger diff between 2019-05-01 to 2018-07-01. The only change is the version name.
The new swagger contains all this, where is this removed?

@ryansbenson ryansbenson added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Jul 12, 2019
@visharm
Copy link
Contributor Author

visharm commented Jul 26, 2019 via email

@majastrz
Copy link
Member

majastrz commented Aug 2, 2019

    "responses": {

You need to add a "default" response to cover all errors in your Swagger. This is missing from new and existing paths. Take a look at blueprint swagger, we have implemented that there.


Refers to: specification/logic/resource-manager/Microsoft.Logic/stable/2019-05-01/logic.json:5110 in 4ee2e9a. [](commit_id = 4ee2e9a, deletion_comment = False)

@majastrz
Copy link
Member

majastrz commented Aug 2, 2019

      }

Your comments suggest that this is an asynchronous/long-running operation, but you are missing x-ms-long-running-operation extension. Please add it to all long running operations.


Refers to: specification/logic/resource-manager/Microsoft.Logic/stable/2019-05-01/logic.json:5280 in 4ee2e9a. [](commit_id = 4ee2e9a, deletion_comment = False)

@majastrz
Copy link
Member

majastrz commented Aug 2, 2019

  "patch": {

Will patch always be synchronous?


Refers to: specification/logic/resource-manager/Microsoft.Logic/stable/2019-05-01/logic.json:5283 in 4ee2e9a. [](commit_id = 4ee2e9a, deletion_comment = False)

@majastrz
Copy link
Member

majastrz commented Aug 2, 2019

    "responses": {

In addition to the default response I mentioned earlier, you should also return 204 when the object doesn't exist. (DELETEs should never return 404.)


Refers to: specification/logic/resource-manager/Microsoft.Logic/stable/2019-05-01/logic.json:5367 in 4ee2e9a. [](commit_id = 4ee2e9a, deletion_comment = False)

@majastrz
Copy link
Member

majastrz commented Aug 2, 2019

  "/{subscriptionId}/resourceGroups/{resourceGroup}/providers/Microsoft.Logic/integrationServiceEnvironments/{integrationServiceEnvironmentName}/health/network": {

The ARM RPC requires paths to follow the format type1/name1/type2/name2. /health/network violates that rule.

I think the best option would be to rename it to networkHealthStatuses or something similar and implement a route to GET a single networkHealthStatuses object. This is the preferred option because it automatically works with RBAC (User with reader role will be able to read status without any issues.). This would also avoid the usage of additionalProperties in the response definition and would allow you to leverage paging when the number of status items gets too big.


Refers to: specification/logic/resource-manager/Microsoft.Logic/stable/2019-05-01/logic.json:5516 in 4ee2e9a. [](commit_id = 4ee2e9a, deletion_comment = False)

Copy link
Member

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

Please fix issues in readme.go.md file

@ArcturusZhang
Copy link
Member

Please fix issues in readme.go.md file

Hi @jhendrixMSFT it seems that there is an issue related with code gen of go, I could not see where the problem locates, could you please help about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.