AppServicePlans.json 2022-09-01- Duplicate model name in spec #23709

jackofallops · 2023-04-25T06:17:01Z

The models for Create (PUT) and Update (PATCH) of Service Plans are different, but named the same making the parsing of the spec non-deterministic. These should have unique names within the spec.

azure-rest-api-specs/specification/web/resource-manager/Microsoft.Web/stable/2022-09-01/AppServicePlans.json

Lines 175 to 183 in b0fe180

    
           { 
        
             "name": "appServicePlan", 
        
             "in": "body", 
        
             "description": "Details of the App Service plan.", 
        
             "required": true, 
        
             "schema": { 
        
               "$ref": "./CommonDefinitions.json#/definitions/AppServicePlan" 
        
             } 
        
           },

azure-rest-api-specs/specification/web/resource-manager/Microsoft.Web/stable/2022-09-01/AppServicePlans.json

Lines 281 to 289 in b0fe180

    
           { 
        
             "name": "appServicePlan", 
        
             "in": "body", 
        
             "description": "Details of the App Service plan.", 
        
             "required": true, 
        
             "schema": { 
        
               "$ref": "#/definitions/AppServicePlanPatchResource" 
        
             } 
        
           },

fixes #23708

ARM API Information (Control Plane)

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

Azure 1st Party Service can try out the Shift Left experience to initiate API design review from ADO code repo. If you are interested, may request engineering support by filling in with the form https://aka.ms/ShiftLeftSupportForm.

Changelog

Add a changelog entry for this PR by answering the following questions:

What's the purpose of the update?
- new service onboarding
- new API version
- update existing version for new feature
- update existing version to fix swagger quality issue in s360
- Other, please clarify
When are you targeting to deploy the new service/feature to public regions? Please provide the date or, if the date is not yet available, the month.
When do you expect to publish the swagger? Please provide date or, the the date is not yet available, the month.
By default, Azure SDKs of all languages (.NET/Python/Java/JavaScript for both management-plane SDK and data-plane SDK, Go for management-plane SDK only ) MUST be refreshed with/after swagger of new version is published. If you prefer NOT to refresh any specific SDK language upon swagger updates in the current PR, please leave details with justification here.

Contribution checklist (MS Employees Only):

I commit to follow the Breaking Change Policy of "no breaking changes"
I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

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

ARM API Review Checklist

Applicability: ⚠️

If your changes encompass only the following scenarios, you should SKIP this section, as these scenarios do not require ARM review.

Change to data plane APIs

Adding new properties

All removals

Otherwise your PR may be subject to ARM review requirements. Complete the following:

Check this box if any of the following apply to the PR so that the label "ARMReview" and "WaitForARMFeedback" will be added by bot to kick off ARM API Review. Missing to check this box in the following scenario may result in delays to the ARM manifest review and deployment.
- Adding a new service
- Adding new API(s)
- Adding a new API version
  -[ ] To review changes efficiently, ensure you copy the existing version into the new directory structure for first commit and then push new changes, including version updates, in separate commits. You can use OpenAPIHub to initialize the PR for adding a new version. For more details refer to the wiki.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
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 you have any breaking changes as defined in the Breaking Change Policy, request approval from the Breaking Change Review Board.

Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Additional details on the process and office hours are on the Breaking Change Wiki.

NOTE: To update API(s) in public preview for over 1 year (refer to Retirement of Previews)

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

fixes #23708

openapi-workflow-bot · 2023-04-25T06:17:04Z

Hi, @jackofallops Thanks for your PR. I am workflow bot for review process. Here are some small tips.

Please ensure to do self-check against checklists in first PR comment.

PR assignee is the person auto-assigned and responsible for your current PR reviewing and merging.

For specs comparison cross API versions, Use API Specs Comparison Report Generator

If there is CI failure(s), to fix CI error(s) is mandatory for PR merging; or you need to provide justification in PR comment for explanation. How to fix?

Any feedback about review process or workflow bot, pls contact swagger and tools team. vscswagger@microsoft.com

openapi-pipeline-app · 2023-04-25T06:17:05Z

Swagger pipeline restarted successfully, please wait for status update in this comment.

openapi-pipeline-app · 2023-04-25T06:17:06Z

Swagger pipeline restarted successfully, please wait for status update in this comment.

openapi-pipeline-app · 2023-04-25T06:17:07Z

Swagger pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

ghost · 2023-04-25T06:17:33Z

Thank you for your contribution jackofallops! We will review the pull request and get back to you soon.

openapi-workflow-bot · 2023-04-25T06:28:05Z

Hi @jackofallops, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review.
Action: To initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
If you want to know the production traffic statistic, please see ARM Traffic statistic.
If you think it is false positive breaking change, please provide the reasons in the PR comment, report to Swagger Tooling Team via https://aka.ms/swaggerfeedback.
Note: To avoid breaking change, you can refer to Shift Left Solution for detecting breaking change in early phase at your service code repository.

openapi-workflow-bot · 2023-04-25T06:28:06Z

Hi @jackofallops, Your PR has some issues. Please fix the CI sequentially by following the order of Avocado, semantic validation, model validation, breaking change, lintDiff. If you have any questions, please post your questions in this channel https://aka.ms/swaggersupport.

Task	How to fix	Priority
Avocado	Fix-Avocado	High
Semantic validation	Fix-SemanticValidation-Error	High
Model validation	Fix-ModelValidation-Error	High
LintDiff	Fix-LintDiff	high

If you need further help, please feedback via swagger feedback.

jackofallops · 2023-04-25T08:06:26Z

@microsoft-github-policy-service agree company="Hashicorp"

v-jiaodi · 2023-04-26T03:18:00Z

@jackofallops Please fill in the correct checkboxes in the first comment so we can get a quick overview of what's going on with this PR and make sure you've understood the relevant linked docs.

v-jiaodi · 2023-04-26T03:18:25Z

/azp run

azure-pipelines · 2023-04-26T03:18:35Z

Azure Pipelines successfully started running 1 pipeline(s).

navba-MSFT · 2023-04-26T04:46:44Z

@jackofallops Please fill in the correct checkboxes in the first comment so we can get a quick overview of what's going on with this PR and make sure you've understood the relevant linked docs.

@jackofallops Could you please follow this suggestion ?

jackofallops · 2023-04-26T05:27:18Z

@jackofallops Please fill in the correct checkboxes in the first comment so we can get a quick overview of what's going on with this PR and make sure you've understood the relevant linked docs.

@jackofallops Could you please follow this suggestion ?

Hi @navba-MSFT - The referenced comment was made at 5am my time (UK), so I've only just seen it. I'm not a Microsoft employee, so do not have access to the links in the first comment, and the section is for MSFT employees only? Can you be more specific about the additional information needed? The issue describes the problem, and this PR is a self-explanatory change to address a violation in the spec, but I'm happy to provide additional info as required or make amendments to the PR as needed?

Thanks!

navba-MSFT · 2023-04-26T05:30:06Z

@jackofallops Please fill in the correct checkboxes in the first comment so we can get a quick overview of what's going on with this PR and make sure you've understood the relevant linked docs.

@jackofallops Could you please follow this suggestion ?

Hi @navba-MSFT - The referenced comment was made at 5am my time (UK), so I've only just seen it. I'm not a Microsoft employee, so do not have access to the links in the first comment, and the section is for MSFT employees only? Can you be more specific about the additional information needed? The issue describes the problem, and this PR is a self-explanatory change to address a violation in the spec, but I'm happy to provide additional info as required or make amendments to the PR as needed?

Thanks!

@jackofallops Please update the PR description here.

jackofallops · 2023-04-26T05:36:54Z

@jackofallops Please fill in the correct checkboxes in the first comment so we can get a quick overview of what's going on with this PR and make sure you've understood the relevant linked docs.

@jackofallops Could you please follow this suggestion ?

Hi @navba-MSFT - The referenced comment was made at 5am my time (UK), so I've only just seen it. I'm not a Microsoft employee, so do not have access to the links in the first comment, and the section is for MSFT employees only? Can you be more specific about the additional information needed? The issue describes the problem, and this PR is a self-explanatory change to address a violation in the spec, but I'm happy to provide additional info as required or make amendments to the PR as needed?
Thanks!

@jackofallops Please update the PR description here.

Hi @navba-MSFT - I'd be happy to. Can you please be specific about what is necessary to be updated? I have completed all I that I am instructed to in the template, and having checked many other open PRs none have any further information than I have provided. Please elaborate.

@jackofallops I have updated the PR description

navba-MSFT · 2023-04-27T04:04:50Z

@v-jiaodi @naveedaz Could you please review this PR once you get a chance ? Thanks in advance.

v-jiaodi · 2023-04-27T06:02:30Z

@JeffreyRichter Could you help take a look at the breaking change of this PR ? Thanks

JeffreyRichter · 2023-04-27T17:21:24Z

Ideally, the models for PUT and PATCH are identical. This way, a customer could create and initialize a struct via an SDK and then pass that to either a PUT or PATCH method. Or call a GET (which returns an initialized struct) and then pass that to PUT or PATCH. It seems to me that you are purposely making the models different.

That being said, if the service has already GA'd then we don't want to introduce a breaking change. So is this change correcting the swagger to make it accurately reflect the currently-shipping service behavior?

weidongxu-microsoft

Comment added. The "name" of body parameter seems cosmetic.

Also, should you target the next api-version release branch? e.g. as #23567

weidongxu-microsoft · 2023-04-28T01:36:25Z

specification/web/resource-manager/Microsoft.Web/stable/2022-09-01/AppServicePlans.json

@@ -279,7 +279,7 @@
            "type": "string"
          },
          {
-            "name": "appServicePlan",
+            "name": "appServicePlanPatch",


the change of "name" on body parameter seems unnecessary, as probably only python uses the "name" (it won't affect REST API -- body is body, but it may break SDK if they indeed uses the "name" of body).

@jackofallops FYI

This makes the SDK generation non-deterministic? If this is used as is, the wrong model will be selected (without some override) in some languages SDKs (Python/Go etc). So the PATCH will use the PUT model here, so this is correcting this. This should have a unique name in this spec as it refers to a different model to the PUT, as @JeffreyRichter called out above:

That being said, if the service has already GA'd then we don't want to introduce a breaking change. So is this change correcting the swagger to make it accurately reflect the currently-shipping service behavior?

Ideally, we'd not want this to refer to another model, as this doesn't match standard behaviour in this and other APIs, but if there's legitimate reason not to, we should update here for the name to be unique?

This is a parameter name, not a model name.

The model is here (which is the correct one AppServicePlanPatchResource?)

azure-rest-api-specs/specification/web/resource-manager/Microsoft.Web/stable/2022-09-01/AppServicePlans.json

Lines 286 to 288 in f0e0f76

"schema": {

"$ref": "#/definitions/AppServicePlanPatchResource"

}

This model will be used whatever the parameter name.

I know Java don't need this change (though it won't break as well; probably same for .NET). API signature would be patch(String, String, ..., AppServicePlanPatchResource), parameter name would not show anywhere.

Add @msyyc for Python, @tadelesh for Go.

@jackofallops If this change is intended for SDK, do talk to SDK owners. I've already added them above.

@jackofallops Could you please let us know the intended SDK ? Depending on that we can tag the right SDK owners.

@navba-MSFT this change is for github.com/hashicorp/go-azure-sdk (so we'd be the owners/folks to tag there, fwiw)

It will not break Azure Go SDK. But it is labeled as a swagger breaking change (though I don't think it will break payload), it's better to get approval from Jeff.

Here's what I think is happening: Today, there is an SDK-only method parameter named appServicePlan and there is desire to change this name to appServicePlanPatch. I'm not sure why this desire exists but the only real language it might affect is C#; not Go. And this has such a small chance of breaking .NET that we'd approve it. But I don't understand the motivation for making this change?

Do I have this right?

v-jiaodi · 2023-06-25T06:22:44Z

/azp run

azure-pipelines · 2023-06-25T06:22:53Z

Azure Pipelines successfully started running 1 pipeline(s).

openapi-pipeline-app · 2024-03-29T05:47:13Z

Next Steps to Merge

Next steps that must be taken to merge this PR:

❌ Your PR has at least one breaking change (label: BreakingChangeReviewRequired). See the PR description for help.
❌ The required check named Swagger BreakingChange has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it. In addition, refer to step 1 in the PR workflow diagram (even if your PR is for data plane, not ARM).

update Patch model name

c36dc98

jackofallops requested a review from naveedaz as a code owner April 25, 2023 06:17

openapi-assignment-bot bot assigned weidongxu-microsoft Apr 25, 2023

openapi-workflow-bot bot added the FixS360 label Apr 25, 2023

ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Apr 25, 2023

ghost added the Web Apps label Apr 25, 2023

azure-sdk mentioned this pull request Apr 25, 2023

[AutoPR sdk/resourcemanager/appservice/armappservice] update Patch model name azure-sdk/azure-sdk-for-go#5511

Closed

AzureRestAPISpecReview added BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required CI-FixRequiredOnFailure resource-manager labels Apr 25, 2023

update examples/PatchAppServicePlan.json for new PATCH model name.

f0e0f76

azure-sdk mentioned this pull request Apr 25, 2023

[AutoPR track2_azure-mgmt-web] update Patch model name azure-sdk/azure-sdk-for-python#3394

Closed

openapi-pipeline-app bot added the CI-BreakingChange-Python-Track2 label Apr 25, 2023

AzureRestAPISpecReview removed the CI-FixRequiredOnFailure label Apr 25, 2023

raych1 requested a review from v-jiaodi April 26, 2023 02:12

raych1 assigned v-jiaodi Apr 26, 2023

navba-MSFT mentioned this pull request Apr 26, 2023

AppServicePlans.json 2022-09-01- Duplicate model name in spec #23708

Open

navba-MSFT changed the title ~~update Patch model name~~ AppServicePlans.json 2022-09-01- Duplicate model name in spec Apr 27, 2023

weidongxu-microsoft reviewed Apr 28, 2023

View reviewed changes

v-jiaodi assigned rkmanda and unassigned v-jiaodi Jun 26, 2023

weidongxu-microsoft removed their assignment Jun 27, 2023

msyyc removed the CI-BreakingChange-Python-Track2 label Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppServicePlans.json 2022-09-01- Duplicate model name in spec #23709

AppServicePlans.json 2022-09-01- Duplicate model name in spec #23709

jackofallops commented Apr 25, 2023 •

edited by navba-MSFT

Loading

openapi-workflow-bot bot commented Apr 25, 2023

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

ghost commented Apr 25, 2023

openapi-workflow-bot bot commented Apr 25, 2023

openapi-workflow-bot bot commented Apr 25, 2023

jackofallops commented Apr 25, 2023

v-jiaodi commented Apr 26, 2023

v-jiaodi commented Apr 26, 2023

azure-pipelines bot commented Apr 26, 2023

navba-MSFT commented Apr 26, 2023

jackofallops commented Apr 26, 2023

navba-MSFT commented Apr 26, 2023

jackofallops commented Apr 26, 2023 •

edited by navba-MSFT

Loading

navba-MSFT commented Apr 27, 2023

v-jiaodi commented Apr 27, 2023

JeffreyRichter commented Apr 27, 2023

weidongxu-microsoft left a comment

weidongxu-microsoft Apr 28, 2023 •

edited

Loading

navba-MSFT Apr 28, 2023

jackofallops Apr 28, 2023

weidongxu-microsoft May 5, 2023 •

edited

Loading

navba-MSFT May 9, 2023

tombuildsstuff May 9, 2023

tadelesh May 10, 2023

JeffreyRichter May 10, 2023

v-jiaodi commented Jun 25, 2023

azure-pipelines bot commented Jun 25, 2023

openapi-pipeline-app bot commented Mar 29, 2024

	{
	"name": "appServicePlan",
	"in": "body",
	"description": "Details of the App Service plan.",
	"required": true,
	"schema": {
	"$ref": "./CommonDefinitions.json#/definitions/AppServicePlan"
	}
	},

	"schema": {
	"$ref": "#/definitions/AppServicePlanPatchResource"
	}

AppServicePlans.json 2022-09-01- Duplicate model name in spec #23709

Are you sure you want to change the base?

AppServicePlans.json 2022-09-01- Duplicate model name in spec #23709

Conversation

jackofallops commented Apr 25, 2023 • edited by navba-MSFT Loading

ARM API Information (Control Plane)

Changelog

Contribution checklist (MS Employees Only):

ARM API Review Checklist

Breaking Change Review Checklist

openapi-workflow-bot bot commented Apr 25, 2023

openapi-pipeline-app bot commented Apr 25, 2023 • edited Loading

openapi-pipeline-app bot commented Apr 25, 2023 • edited Loading

openapi-pipeline-app bot commented Apr 25, 2023 • edited Loading

ghost commented Apr 25, 2023

openapi-workflow-bot bot commented Apr 25, 2023

openapi-workflow-bot bot commented Apr 25, 2023

jackofallops commented Apr 25, 2023

v-jiaodi commented Apr 26, 2023

v-jiaodi commented Apr 26, 2023

azure-pipelines bot commented Apr 26, 2023

navba-MSFT commented Apr 26, 2023

jackofallops commented Apr 26, 2023

navba-MSFT commented Apr 26, 2023

jackofallops commented Apr 26, 2023 • edited by navba-MSFT Loading

navba-MSFT commented Apr 27, 2023

v-jiaodi commented Apr 27, 2023

JeffreyRichter commented Apr 27, 2023

weidongxu-microsoft left a comment

Choose a reason for hiding this comment

weidongxu-microsoft Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

navba-MSFT Apr 28, 2023

Choose a reason for hiding this comment

jackofallops Apr 28, 2023

Choose a reason for hiding this comment

weidongxu-microsoft May 5, 2023 • edited Loading

Choose a reason for hiding this comment

navba-MSFT May 9, 2023

Choose a reason for hiding this comment

tombuildsstuff May 9, 2023

Choose a reason for hiding this comment

tadelesh May 10, 2023

Choose a reason for hiding this comment

JeffreyRichter May 10, 2023

Choose a reason for hiding this comment

v-jiaodi commented Jun 25, 2023

azure-pipelines bot commented Jun 25, 2023

openapi-pipeline-app bot commented Mar 29, 2024

Next Steps to Merge

jackofallops commented Apr 25, 2023 •

edited by navba-MSFT

Loading

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 25, 2023 •

edited

Loading

jackofallops commented Apr 26, 2023 •

edited by navba-MSFT

Loading

weidongxu-microsoft Apr 28, 2023 •

edited

Loading

weidongxu-microsoft May 5, 2023 •

edited

Loading