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.

Sign up for GitHub

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

Jump to bottom

[Logic App Integration Account][Bug Fix] : Swagger spec updated as per the latest changes in the RP … #891

Merged
merged 8 commits into from
Feb 14, 2017
Merged

[Logic App Integration Account][Bug Fix] : Swagger spec updated as per the latest changes in the RP … #891

merged 8 commits into from
Feb 14, 2017

Conversation

pankajsn
Copy link
Contributor

@pankajsn pankajsn commented Jan 30, 2017 •
edited
Loading

…definitions.

Issue fixed: As the LogicApps and Integration Account are in GA, swagger spec is merged
Issue fixed: HashingAlgorithm enum in Azure SDK swagger spec is not updated with new values.
Issue fixed: Not able to create New-AzureRmIntegrationAccountAgreement with envelopeOverrides/responsibleAgencyCode due to RP definition updates.
Issue fixed: Integration Account Sku name values fixed as per review comments.

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.
    • Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
    • The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

@pankajsn
Copy link
Contributor Author

pankajsn commented Jan 30, 2017 •
edited
Loading

@jhendrixMSFT I am seeing 110 validation errors regarding property description in the spec. We are not describing them while defining the object but while referring. Please let us know if we need to fix this. Our current change is only few bug fixes.

@pankajsn pankajsn changed the title [Bug Fix] : Swagger spec updated as per the latest changes in the RP … [Logic App Integration Account][Bug Fix] : Swagger spec updated as per the latest changes in the RP … Jan 31, 2017
@pankajsn pankajsn mentioned this pull request Jan 31, 2017
Merged
8 tasks
@jhendrixMSFT
Copy link
Member

Hi @pankajsn I'm following up on your question and should have an answer for soon.

@jhendrixMSFT
Copy link
Member

@pankajsn sorry for the delay. We'll need these validation errors to be fixed, the reason being that they're used for generating docs during SDK generation.

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 3, 2017

@jhendrixMSFT validation errors are fixed.

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 3, 2017 via email

@pankajsn pankajsn closed this Feb 3, 2017
@pankajsn pankajsn reopened this Feb 3, 2017
amarzavery
amarzavery reviewed Feb 6, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json
"KeyType": {
"type": "string",
"enum": [
"NotSpecified",
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change on several counts.

  1. New value is added to the list
  2. New value is added at the top of the list, instead of it being in the bottom. Previous version had Primary and Secondary.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this hasn't been addressed yet?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this value was already there. @amarzavery did you mean a different enum?

@amarzavery amarzavery closed this Feb 6, 2017
@amarzavery amarzavery reopened this Feb 6, 2017
@amarzavery
Copy link
Contributor

@pankajsn - We have hooked up a bunch of validation tools as a part of our CI .

The linter shows some warnings. Please make sure you address them before the PR can be merged.
Warnings can be found here.

  AutoRest Linter validation:
Executing: mono ./AutoRest.*/tools/AutoRest.exe -CodeGenerator None -I arm-logic/2016-06-01/swagger/logic.json
WARNING: NonAppJsonTypeWarning - Media types other than 'application/json' has limited support
	Path: #/Consumes[2]
WARNING: NonAppJsonTypeWarning - Media types other than 'application/json' has limited support
	Path: #/Produces[2]
WARNING: OperationsAPIImplementationValidation - Operations API must be implemented for the service.
	Path: #/Paths
WARNING: ResourceModelValidation - The id, name, type, location and tags properties of the Resource must be present with id, name and type as read-only
	Path: #/Definitions
WARNING: TrackedResourceValidation - Tracked Resource failing validation is: Workflow. Validation Failed: 4. 
    A Tracked Resource must have: 
    1. A Get Operation 
    2. A ListByResourceGroup operation with x-ms-pageable extension and 
    3. A ListBySubscriptionId operation with x-ms-pageable extension.
    4. Type, Location, Tags should not be used in the properties.
	Path: #/Definitions
WARNING: SkuModelValidation - Sku Model is not valid. A Sku model must have name property. It can also have tier, size, family, capacity as optional properties.
	Path: #/Definitions

@amarzavery
Copy link
Contributor

amarzavery commented Feb 6, 2017 •
edited
Loading

Hi @pankajsn,

Can you please provide examples for request and response using the extension x-ms-examples? This will

  • help us validate the examples against the swagger spec and
  • it will also form the basis for customer facing REST API documentation on docs.microsoft.com website.

You can find more info about the extension and an example swagger spec over here.

amarzavery
amarzavery reviewed Feb 7, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json
"description": "No Content"
}
}
}
}
},
"definitions": {
"Resource": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pankajsn There is an issue #898 reported by a customer which is correct. Can you please resolve this issue, by accurately representing the properties of "Resource" "readOnly": true.

  • "id", "name" and "type" should be marked "readOnly": true as per ARM guidelines.
  • Shouldn't "location" be a required property "required": ["location"]?

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 7, 2017 via email

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Feb 7, 2017 •
edited
Loading

I sat down with @amarzavery regarding your questions, below are the answers. Let me know if you have further questions.

NonAppJsonTypeWarning - can you please verify that your RP actually consumes and produces text/json in addition to application/json?

OperationsAPIImplementationValidation - you need to implement the operations API. For an example please see https://github.com/Azure/azure-rest-api-specs/blob/master/arm-cdn/2016-10-02/swagger/cdn.json#L1462

TrackedResourceValidation - the output here is unclear and we will work to fix that. Only item 4 (type, location etc) has an issue (that's what the "Validation Failed: 4" means) and this appears to be a bug in the validator. You can ignore this for now.

SkuModelValidation - another unclear error message which we'll address. The "name" field needs to be marked as required.

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 8, 2017 via email

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 8, 2017 via email

amarzavery
amarzavery suggested changes Feb 8, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
"tags": [
"IntegrationAccountSchemas"
],
"operationId": "IntegrationAccountSchemas_List",
Copy link
Contributor

Choose a reason for hiding this comment

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

The current version of 2016-06-01 api-version does not have these APIs in the public repo. Hence they are new. I would like them to follow the naming convention right from the start, so that there are no breaking changes w.r.t naming.

For nested resources the convention is: _ListByImmediateParent.
So this should be:
Schemas_ListByIntegrationAccounts. This will make things consistent with other APIs in Azure. The customer need not learn new things when he moves from one RP to another in Azure.

For reference: Here is the convention for naming

M1004: List operations MUST use consistent names and semantics. List operations MUST NOT use any other names. Consistent names and semantics are:

- List() - lists all resources under a subscription.
- ListByResourceGroup() - list all resources in a resource group within a subscription.
- ListByParent() - where "Parent" is a context specific suffix. It lists all resource under a parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

arm-logic/2016-06-01/swagger/logic.json Outdated
"tags": [
"IntegrationAccountSchemas"
],
"operationId": "IntegrationAccountSchemas_Get",
Copy link
Contributor

Choose a reason for hiding this comment

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

General comment: The method Group (part before the underscore) should match with the nested resource type in the url.
Schemas_Get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

arm-logic/2016-06-01/swagger/logic.json Outdated
"tags": [
"IntegrationAccountMaps"
],
"operationId": "IntegrationAccountMaps_List",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maps_ListByIntegrationAccounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

arm-logic/2016-06-01/swagger/logic.json Outdated
"tags": [
"IntegrationAccountMaps"
],
"operationId": "IntegrationAccountMaps_Get",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: General comment: Maps_Get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

arm-logic/2016-06-01/swagger/logic.json Outdated
"tags": [
"IntegrationAccountPartners"
],
"operationId": "IntegrationAccountPartners_List",
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

required properties?

arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

enum?

arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

any required or readOnly properties over 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.

fixed across

arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you expect the customer to put over here? Is "FooBar" valid? Can the descriptions be more meaningful with some example values.

arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

General comment please provide required and readOnly properties of all the model definitions accurately. It is hard to believe that everything is optional and free flowing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed accross

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 8, 2017 via email

@jhendrixMSFT
Copy link
Member

@pankajsn If there's a tight deadline then perhaps it makes sense to update this PR (or close this one and create a new one, whichever is easier) with only the necessary bug fixes for PowerShell (IIRC it was just a few lines of changes), and do this larger refactoring/API additions in a separate PR. Thoughts?

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 8, 2017 via email

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 8, 2017 via email

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 9, 2017 via email

@amarzavery
Copy link
Contributor

@pankajsn - I am sorry, cannot do anything about it. It is a github issue. As per github guidelines, it is expected that the author resolves all the comments made by the reviewer before making a new commit. With the new commit the line numbers change and nothing can be done about it.

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to this thing "content". What is the schema? Is it free form?

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the qualifier? Is this an enum? How would the customer know what to provide?

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide an example in the description of qualifier and value

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any required properties in "AS2OneWayAgreement"?

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any required properties in "AS2ProtocolSettings"? I mean if the user provides an empty object what would the service do? Accept it and apply defaults or send a 400?

amarzavery
amarzavery reviewed Feb 10, 2017
View reviewed changes
arm-logic/2016-06-01/swagger/logic.json Outdated
},
"Correlation": {
"type": "object",

Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. The service is having huge model definitions. But everything is optional. It looks like garbage in garbage out. There is no way to know what should be provided on a bear minimum level

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 11, 2017 via email

@pankajsn
Copy link
Contributor Author

pankajsn commented Feb 13, 2017 via email

@jhendrixMSFT
Copy link
Member

@amarzavery I believe you're good with the current iteration, if so can you please sign off on this PR so we can merge it?

@amarzavery amarzavery merged commit c0a3a59 into Azure:master Feb 14, 2017
@AutorestCI
Copy link

No modification for Ruby

@AutorestCI
Copy link

No modification for Python

@AutorestCI
Copy link

No modification for NodeJS

@jhendrixMSFT jhendrixMSFT mentioned this pull request Feb 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhendrixMSFT jhendrixMSFT jhendrixMSFT left review comments

@amarzavery amarzavery amarzavery approved these changes

Assignees

@jhendrixMSFT jhendrixMSFT

Labels
Logic App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pankajsn @jhendrixMSFT @amarzavery @AutorestCI @kirthik @azurecla