Skip to content

Managed Services General API Fixes#9886

Merged
akning-ms merged 4 commits intoAzure:masterfrom
skayani:managedServicesGeneralFixes
Jul 7, 2020
Merged

Managed Services General API Fixes#9886
akning-ms merged 4 commits intoAzure:masterfrom
skayani:managedServicesGeneralFixes

Conversation

@skayani
Copy link
Copy Markdown
Contributor

@skayani skayani commented Jun 18, 2020

-For stable/2019-09-01 and preview/2020-02-01, added MarketplaceRegistration property and use it in place of RegistrationDefinition for MarketplaceRegistrationDefinitionsList
-Note for the above change, from the very beginning MarketplaceRegistrationDefinitionsList should have used MarketplaceRegistrationDefinition so this was a mistake fix.
-For preview/2020-02-01, added eligibleAuthorizations to GetMarketplaceRegistrationDefinition and GetMarketplaceRegistrationDefinitionsList examples

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

  • 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.

skayani added 2 commits June 18, 2020 13:48
MarketplaceRegistration property and use it in place of
RegistrationDefinition for MarketplaceRegistrationDefinitionsList
-For preview/2020-02-01, added eligibleAuthorizations to
GetMarketplaceRegistrationDefinition and
GetMarketplaceRegistrationDefinitionsList examples
@openapi-pipeline-app
Copy link
Copy Markdown

openapi-pipeline-app bot commented Jun 18, 2020

[Staging] Swagger Validation Report

BreakingChange: 14 Errors, 0 Warnings [Detail] [Expand] Only 10 items are listed, please refer to log for more details.
Rule Message
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'description' renamed or removed?
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'registrationDefinitionName' renamed or removed?
New: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L798:7
Old: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'provisioningState' renamed or removed?
New: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L798:7
Old: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'managedByTenantName' renamed or removed?
New: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L798:7
Old: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'provisioningState' renamed or removed?
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'managedByTenantName' renamed or removed?
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'registrationDefinitionName' renamed or removed?
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
1033 - RemovedProperty The new version is missing a property found in the old version. Was 'description' renamed or removed?
New: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L798:7
Old: Microsoft.ManagedServices/stable/2019-09-01/managedservices.json#L530:7
1041 - AddedPropertyInResponse The new version has a new property 'offerDisplayName' in response that was not found in the old version.
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
1041 - AddedPropertyInResponse The new version has a new property 'publisherDisplayName' in response that was not found in the old version.
New: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L812:7
Old: Microsoft.ManagedServices/preview/2020-02-01-preview/managedservices.json#L530:7
️✔️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
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 2020

azure-sdk-for-go - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 2020

azure-sdk-for-net - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 2020

azure-sdk-for-java - Release

⚠️ warning [Logs] [Expand Details]
  • ⚠️ Generate from 9a93ba5 with merge commit 3657724. SDK Automation 13.0.17.20200619.4
    Failed to find any diff after autorest so no changed packages was found.

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 2020

azure-sdk-for-python - Release

️✔️ succeeded [Logs] [Expand Details]
  • ️✔️ Generate from 9a93ba5 with merge commit 3657724. SDK Automation 13.0.17.20200619.4
  • ️✔️azure-mgmt-managedservices [View full logs]  [Release SDK Changes]
    [build_conf] INFO:packaging_tools:Building template azure-mgmt-managedservices
    [build_conf] INFO:packaging_tools.conf:Skipping default conf since the file exists
    [build_conf] INFO:packaging_tools:Skipping CHANGELOG.md template, since a previous one was found
    [build_conf] INFO:packaging_tools:Template done azure-mgmt-managedservices
    [build_package] /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'long_description_content_type'
    [build_package]   warnings.warn(msg)
    [build_package] /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'long_description_content_type'
    [build_package]   warnings.warn(msg)
    [breaking_change_setup] Ignoring mock: markers 'python_version <= "2.7"' don't match your environment
    [ChangeLog] Size of delta 32.946% size of original (original: 13009 chars, delta: 4286 chars)
    [ChangeLog] **Features**
    [ChangeLog] 
    [ChangeLog]   - Model RegistrationDefinitionProperties has a new parameter eligible_authorizations
    [ChangeLog]   - Model Authorization has a new parameter delegated_role_definition_ids
    [ChangeLog]   - Model Authorization has a new parameter principal_id_display_name
    [ChangeLog]   - Model RegistrationAssignmentPropertiesRegistrationDefinitionProperties has a new parameter eligible_authorizations
    [ChangeLog]   - Added operation group MarketplaceRegistrationDefinitionsOperations

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 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
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 2020

azure-sdk-for-js - Release

⚠️ warning [Logs] [Expand Details]
  • ⚠️ Generate from 9a93ba5 with merge commit 3657724. SDK Automation 13.0.17.20200619.4
    Failed to find any diff after autorest so no changed packages was found.

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 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

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Jun 18, 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

@skayani
Copy link
Copy Markdown
Contributor Author

skayani commented Jun 18, 2020

Although those properties are being tagged as 'removed,' our API does not use those properties to begin with.

@azuresdkci
Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@erich-wang erich-wang added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 19, 2020
@erich-wang
Copy link
Copy Markdown
Member

@skayani , could you please take a look the ModelValidation error in CI?

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@skayani
Copy link
Copy Markdown
Contributor Author

skayani commented Jun 19, 2020

@skayani , could you please take a look the ModelValidation error in CI?

@erich-wang , Thanks for the heads up. It seems the only failing tests now our the BreakingChanges, as to be expected. Let me know if you need any other info.

@ravbhatnagar
Copy link
Copy Markdown
Contributor

Removing ARM Feedback label since ARM review board will not be reviewing property level changes going forward.

@ravbhatnagar ravbhatnagar removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 23, 2020
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@skayani
Copy link
Copy Markdown
Contributor Author

skayani commented Jun 26, 2020

@erich-wang Friendly ping, any update?

@erich-wang
Copy link
Copy Markdown
Member

@skayani, you're introducing breaking change to stable version. What do you mean mistake fix? Do you mean the previous swagger spec never works?

@skayani
Copy link
Copy Markdown
Contributor Author

skayani commented Jun 29, 2020

@erich-wang The API contract is incorrectly stated. For the GetMarketplaceRegistrationDefinition and GetMarketplaceRegistrationDefintions calls, a MarketplaceRegistrationDefinition object is returned not RegistrationDefinition. This issue was found internally since we don't have consumers for these Swagger specs yet.

@erich-wang
Copy link
Copy Markdown
Member

@akning-ms , could you please force merge this PR as service team confirms that there's no customers for the API with breaking change.

@erich-wang
Copy link
Copy Markdown
Member

As identified in the automated checks there are breaking changes, Please follow Azure Breaking changes approval request form as defined @ Breaking Change Process

@skayani
Copy link
Copy Markdown
Contributor Author

skayani commented Jul 1, 2020

@erich-wang Hi Erich, I just wanted to clarify before proceeding with that process. As mentioned before, our Managed Services RP API contract has not changed at all, it's the Swagger specs that are incorrect, which is what this PR fixes. For example, for retrieving Marketplace Registration Definitions, our API contract has always returned a Marketplace Registration Definition property. In the Swagger specs currently however, it states a Registration Definition property is returned, which is incorrect. Additionally, we don't have customers using the marketplace registration definitions currently (if we did it would be broken due to this mistake). Knowing that, do we still need to fill out the breaking change approval form?

@erich-wang
Copy link
Copy Markdown
Member

@erich-wang Hi Erich, I just wanted to clarify before proceeding with that process. As mentioned before, our Managed Services RP API contract has not changed at all, it's the Swagger specs that are incorrect, which is what this PR fixes. For example, for retrieving Marketplace Registration Definitions, our API contract has always returned a Marketplace Registration Definition property. In the Swagger specs currently however, it states a Registration Definition property is returned, which is incorrect. Additionally, we don't have customers using the marketplace registration definitions currently (if we did it would be broken due to this mistake). Knowing that, do we still need to fill out the breaking change approval form?

@akning-ms, could you force merge this PR given the breaking change is bug fix?

@erich-wang erich-wang added Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374 and removed BreakingChangeReviewRequired labels Jul 6, 2020
@akning-ms akning-ms merged commit 3657724 into Azure:master Jul 7, 2020
00Kai0 pushed a commit to 00Kai0/azure-rest-api-specs that referenced this pull request Oct 12, 2020
* -For stable/2019-09-01 and preview/2020-02-01, added
MarketplaceRegistration property and use it in place of
RegistrationDefinition for MarketplaceRegistrationDefinitionsList
-For preview/2020-02-01, added eligibleAuthorizations to
GetMarketplaceRegistrationDefinition and
GetMarketplaceRegistrationDefinitionsList examples

* Removed extraneous spacing

* Fixed get marketplace reg def examples to reference MarketplaceRegDef property

* Added name property to marketplace reg def + fixed examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants