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

Add 6 lint rules feedback by ARM to the guideline #10125

Merged
merged 11 commits into from
Aug 5, 2020
Merged

Conversation

jianyexi
Copy link
Contributor

@jianyexi jianyexi commented Jul 14, 2020

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

Contribution checklist:

If any further question about AME onboarding or validation tools, please view the FAQ.

ARM API Review Checklist

  • Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.

    • Adding new API(s)
    • Adding a new API version
    • Adding a new service
  • If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist

If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

  • Removing API(s) in stable version
  • Removing properties in stable version
  • Removing API version(s) in stable version
  • Updating API in stable version with Breaking Change Validation errors
  • Updating API(s) in preview over 1 year

Please follow the link to find more details on PR review process.

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jul 14, 2020

[Staging] Swagger Validation Report

️✔️BreakingChange [Detail]
 There are no breaking changes. 
️✔️LintDiff [Detail]
 Validation passes for LintDiff. 
️✔️Avocado [Detail]
 Validation passes for Avocado. 
Posted by Swagger Pipeline | How to fix these errors?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

Azure CLI Extension Generation - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-go - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-java - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-js - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

Trenton Generation - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-net - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-python - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 14, 2020

azure-sdk-for-python-track2 - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@akning-ms akning-ms requested review from pilor and KrisBash July 14, 2020 06:19
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


**Output Message** : The response in the GET collection operation "{0}" is different than the response definition in the individual GET operation "{1}".

**Description** : For all resources (top-level and nested), collection GETs should have a response definition with a single property "value" containing an array of the "resource" schema.The definition returned in the collection "value" array should be the same as the response body for the individual GET.
Copy link
Contributor

Choose a reason for hiding this comment

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

response definition with a single property "value" [](start = 90, length = 50)

this isn't technically true, the definition should contain a "nextLink" property as well in most cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks,I removed the word "single" to avoid the confusion

Copy link
Contributor

@pilor pilor left a comment

Choose a reason for hiding this comment

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

One minor comment

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jianyexi
Copy link
Contributor Author

Can other reviewers take a look ?

@jianyexi
Copy link
Contributor Author

@josefree could you review this PR ?

@jianyexi
Copy link
Contributor Author

@ravbhatnagar @KrisBash @chiragg4u @majastrz Could you also review this PR to see if the rules match your requirement ? thanks

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


### <a name="r4014" ></a>R4014 AllResourcesMustHaveGetOperation

**Category** : ARM Warning
Copy link
Contributor

@akning-ms akning-ms Jul 23, 2020

Choose a reason for hiding this comment

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

As this Rule is mainly to detect resource model which don't have path/get operation. As it is hard to know whether it is a really resource type from model, so we put it a warning first. @ ARM to confirm

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 want me to confirm? If there is a PUT or a LIST on a URI there should be an equivalent GET for individual resources at that same path (i.e. GET on the PUT's URI, or GET on the LIST + {name} URI). This doesn't seem like it should be driven off the definitions, it should be driven off the paths in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pilor, yes we want to confirm the approach to distinguish resourceType in path. your idea looks good.
but Is there any exception? e.g. /operations. if no ideal solution, we may put this rule as warn, not error initially

Copy link
Contributor

@pilor pilor Jul 24, 2020

Choose a reason for hiding this comment

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

/operations and /skus should be the only legitimate exceptions, good call on that. I'm sure there are plenty of places where RPs have done this but it should be something that requires a suppression in order to use since it goes against the RPC. Is there a way to run the rule against all existing specs to see what the current problem space is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we already scan the whole specs, and over 300 violations of this rule, a lot of them only have list model operation and no put operation and point get operation , so it's hard to say that these models are ARM resource, as they can be some kinds of operations' result

@openapi-assignment-bot openapi-assignment-bot bot added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jul 23, 2020
@@ -56,6 +56,11 @@ We request OpenAPI(Swagger) spec authoring be assigned to engineers who have an
| [R4007](#r4007) | [DefaultErrorResponseSchema](#r4007) | ARM OpenAPI(swagger) specs |
| [R4010](#r4010) | [RequiredDefaultResponse](#r4010) | ARM OpenAPI(swagger) specs |
| [R4011](#r4011) | [DeleteOperationResponses](#r4011) | ARM OpenAPI(swagger) specs |
| [R4015](#r4015) | [NestedResourcesMustHaveListOperation](#r4015) | ARM OpenAPI(swagger) specs |
| [R4016](#r4016) | [TopLevelResourcesListByResourceGroup](#r4016) | ARM OpenAPI(swagger) specs |
Copy link
Contributor

Choose a reason for hiding this comment

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

this, i believe, is already automated.

@@ -56,6 +56,11 @@ We request OpenAPI(Swagger) spec authoring be assigned to engineers who have an
| [R4007](#r4007) | [DefaultErrorResponseSchema](#r4007) | ARM OpenAPI(swagger) specs |
| [R4010](#r4010) | [RequiredDefaultResponse](#r4010) | ARM OpenAPI(swagger) specs |
| [R4011](#r4011) | [DeleteOperationResponses](#r4011) | ARM OpenAPI(swagger) specs |
| [R4015](#r4015) | [NestedResourcesMustHaveListOperation](#r4015) | ARM OpenAPI(swagger) specs |
| [R4016](#r4016) | [TopLevelResourcesListByResourceGroup](#r4016) | ARM OpenAPI(swagger) specs |
| [R4017](#r4017) | [TopLevelResourcesListBySubscription](#r4017) | ARM OpenAPI(swagger) specs |
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be already automated.

Copy link
Contributor Author

@jianyexi jianyexi Jul 28, 2020

Choose a reason for hiding this comment

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

Did you mean these rules ?:
R3027 TrackedResourceListByResourceGroup,
R3028 TrackedResourceListBySubscription
R3010 TrackedResourceListByImmediateParent

They only deal with tracked resources while the new rules will consider other resources

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jul 27, 2020
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akning-ms akning-ms merged commit 0b0c022 into master Aug 5, 2020
@jianyexi jianyexi deleted the update_lintdoc branch December 18, 2020 07:29
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.

6 participants