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

Exclude GET verb from LroExtension rule validation #616

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

AkhilaIlla
Copy link
Collaborator

@AkhilaIlla AkhilaIlla commented Nov 1, 2023

#549

This PR contains updates to
RPC-GET-V1-01(GetOperation200): to allow GET with 202 response code and
RPC-Post-V1-09(LroExtension): to exclude GET from x-ms-long-running-operation presence validation
in order to support pooling kinda API's

"/providers/Microsoft.Subscription/subscriptionOperations/{operationId}": {
      "get": {
        "description": "Get the status of the pending Microsoft.Subscription API operations.",
        "operationId": "SubscriptionOperation_Get",
        "x-ms-examples": {
          "getPendingSubscriptionOperations": {
            "$ref": "./examples/getSubscriptionOperation.json"
          }
        },
        "responses": {
          "200": {
            "description": "Successful completion of the asynchronous operation",
            "schema": {
              "$ref": "#/definitions/SubscriptionCreationResult"
            }
          },
          "202": {
            "description": "Accepted. Subscription update is in progress.",
            "headers": {
              "Location": {
                "description": "The URL where the status of the asynchronous operation can be checked.",
                "type": "string"
              },
              "Retry-After": {
                "description": "The amount of delay to use while the status of the operation is checked. The value is expressed in seconds.",
                "type": "integer",
                "format": "int64"
              }
            }
          },
          "default": {
            "description": "Error response describing why the operation failed.",
            "schema": {
              "$ref": "../../../../../common-types/resource-management/v5/types.json#/definitions/ErrorResponse"
            }
          }
        },
        "parameters": [
          {
            "name": "operationId",
            "in": "path",
            "description": "The operation ID, which can be found from the Location field in the generate recommendation response header.",
            "required": true,
            "type": "string"
          },
          {
            "$ref": "#/parameters/apiVersionParameter"
          }
        ]
      }
    },

akhilailla added 2 commits November 1, 2023 14:21
…code and RPC-Post-V1-09(LroExtension) to exclude GET from x-ms-long-running-operation presence validation
- RPC-Get-V1-01

## Output Message

Copy link
Member

@rkmanda rkmanda Nov 7, 2023

Choose a reason for hiding this comment

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

Lets use the new template that was decided for the help files. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

cant find the link where this was documented. Please check with Tejaswi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

July 07, 2022

## How to fix the violation

Copy link
Member

@rkmanda rkmanda Nov 7, 2023

Choose a reason for hiding this comment

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

include good and bad examples #Closed

{
"packageName": "@microsoft.azure/openapi-validator-rulesets",
"comment": "Update RPC-GET-V1-01(GetOperation200) to allow GET with 202 response code and RPC-Post-V1-09(LroExtension) to exclude GET from x-ms-long-running-operation presence validation",
"type": "patch"
Copy link
Member

@rkmanda rkmanda Nov 7, 2023

Choose a reason for hiding this comment

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

  "comment": "Update RPC-GET-V1-01(GetOperation200) to allow GET with 202 response code if the GET represents the location header polling url. Update RPC-Post-V1-09(LroExtension) to exclude GET from x-ms-long-running-operation presence validation",

#Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didnt add
"if the GET represents the location header polling url" cause that isnt being checked in this rule

@rkmanda
Copy link
Member

rkmanda commented Nov 7, 2023

        description: "Operations with a 202 response must specify `x-ms-long-running-operation: true`.",

Make it explicit everywhere that this does not apply to Get including the help file #Closed


Refers to: packages/rulesets/generated/spectral/az-arm.js:891 in 290a9a6. [](commit_id = 290a9a6, deletion_comment = False)

// already have rule to check if operation returns non 2XX, it should mark it as 'x-ms-error-response' explicitly
// https://github.com/Azure/azure-openapi-validator/issues/549
// GET can return 202 only if it is a polling action & has Location header defined. LroLocationHeader rule already checks if 202 response has Location header
// so here just check for non 200, 202 response codes i.e, '201','203','204'
// RPC Code: RPC-Get-V1-01
Copy link
Member

@rkmanda rkmanda Nov 7, 2023

Choose a reason for hiding this comment

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

why not make it an inclusion list for 200, 202 instead of an exclusion list for 201, 203, 204? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

That way we get broader coverage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thats a good point!
But we'd not be able to use the core function

Copy link
Member

@rkmanda rkmanda left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Member

@rkmanda rkmanda left a comment

Choose a reason for hiding this comment

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

:shipit:

@AkhilaIlla AkhilaIlla merged commit cee3e29 into main Nov 10, 2023
4 checks passed
@AkhilaIlla AkhilaIlla deleted the akhilailla/fix_RPC-Post-V1-09_and_RPC-Get-V1-01 branch November 10, 2023 18:39
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.

2 participants