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 support for default encryption scope & block encryption scope override for blob container APIs. #8595

Merged
merged 5 commits into from
Mar 26, 2020
Merged

Conversation

tonykjose
Copy link
Contributor

@tonykjose tonykjose commented Mar 4, 2020

…rride for blob container APIs.

Latest improvements:

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

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

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.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

5 similar comments
@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).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Mar 4, 2020

azure-sdk-for-java - Release

⚠️ warning [Logs] [Expand Details]

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Mar 4, 2020

azure-sdk-for-go - Release

⚠️ warning [Logs] [Expand Details]
  • ⚠️ Generate from f31b309 with merge commit 9fb516a. SDK Automation 13.0.17.20200326.3
  • ⚠️preview/storage/mgmt/2015-05-01-preview [Logs
      No file is changed.
    • ⚠️preview/storage/mgmt/2018-03-01-preview [Logs
        No file is changed.
      • ⚠️storage/mgmt/2015-06-15 [Logs
          No file is changed.
        • ⚠️storage/mgmt/2016-01-01 [Logs
            No file is changed.
          • ⚠️storage/mgmt/2016-05-01 [Logs
              No file is changed.
            • ⚠️storage/mgmt/2016-12-01 [Logs
                No file is changed.
              • ⚠️storage/mgmt/2017-06-01 [Logs
                  No file is changed.
                • ⚠️storage/mgmt/2017-10-01 [Logs
                    No file is changed.
                  • ⚠️storage/mgmt/2018-02-01 [Logs
                      No file is changed.
                    • ⚠️storage/mgmt/2018-07-01 [Logs
                        No file is changed.
                      • ⚠️storage/mgmt/2018-11-01 [Logs
                          No file is changed.
                        • ⚠️storage/mgmt/2019-04-01 [Logs
                            No file is changed.
                          • ️✔️storage/mgmt/2019-06-01 [Logs]  [Release SDK Changes]

                          @openapi-sdkautomation
                          Copy link

                          openapi-sdkautomation bot commented Mar 4, 2020

                          azure-sdk-for-js - Release

                          ️✔️ succeeded [Logs] [Expand Details]

                          @openapi-sdkautomation
                          Copy link

                          openapi-sdkautomation bot commented Mar 4, 2020

                          azure-sdk-for-net - Release

                          failed [Logs] [Expand Details]

                          @openapi-sdkautomation
                          Copy link

                          openapi-sdkautomation bot commented Mar 4, 2020

                          azure-sdk-for-python - Release

                          - Breaking Change detected in SDK

                          ⚠️ warning [Logs] [Expand Details]
                          • ⚠️ Generate from f31b309 with merge commit 9fb516a. SDK Automation 13.0.17.20200326.3
                          • ⚠️azure-mgmt-storage [Logs]  [Release SDK Changes] Breaking Change Detected
                            [after_scripts|python] WARNING:__main__:Found too much API version: {'2018-03-01-preview', '2018-07-01'} in label v2018_07_01
                            [after_scripts|python] WARNING:__main__:Guessing you want 2018-07-01 based on label v2018_07_01
                            [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
                            [breaking_change_setup] Cannot uninstall requirement azure-nspkg, not installed
                            [breaking_change_setup] Command '['/usr/local/bin/python', '-m', 'pip', 'uninstall', '-y', 'azure-nspkg']' returned non-zero exit status 1.
                            [ChangeLog] Size of delta 2.101% size of original (original: 71101 chars, delta: 1494 chars)
                            [ChangeLog] **Features**
                            [ChangeLog] 
                            [ChangeLog]   - Model ListContainerItem has a new parameter default_encryption_scope
                            [ChangeLog]   - Model ListContainerItem has a new parameter deny_encryption_scope_override
                            [ChangeLog]   - Model KeyVaultProperties has a new parameter last_key_rotation_timestamp
                            [ChangeLog]   - Model KeyVaultProperties has a new parameter current_versioned_key_identifier
                            [ChangeLog]   - Model BlobContainer has a new parameter default_encryption_scope
                            [ChangeLog]   - Model BlobContainer has a new parameter deny_encryption_scope_override
                            [ChangeLog] 
                            [ChangeLog] **Breaking changes**
                            [ChangeLog] 
                            [ChangeLog]   - Operation BlobContainersOperations.update has a new signature
                            [ChangeLog]   - Operation BlobContainersOperations.create has a new signature
                            [ChangeLog]   - Operation BlobContainersOperations.update has a new signature
                            [ChangeLog]   - Operation BlobContainersOperations.create has a new signature

                          This was referenced Mar 4, 2020
                          @azuresdkci
                          Copy link
                          Contributor

                          Can one of the admins verify this patch?

                          "type": "string",
                          "description": "Default the container to use specified encryption scope for all writes."
                          },
                          "denyEncryptionScopeOverride": {
                          Copy link
                          Member

                          @qianwens qianwens Mar 9, 2020

                          Choose a reason for hiding this comment

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

                          denyEncryptionScopeOverride [](start = 9, length = 27)

                          Consider to to use enum or string instead of boolean in case you need other mode which may cause breaking change.

                          Copy link
                          Contributor Author

                          Choose a reason for hiding this comment

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

                          This was intentionally modeled as a boolean to match data plane headers following the same.

                          @qianwens qianwens added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Mar 10, 2020
                          @qianwens
                          Copy link
                          Member

                          Invite ARM review since this PR add properties in the put request.

                          @azure-pipelines
                          Copy link

                          Azure Pipelines successfully started running 1 pipeline(s).

                          @openapi-sdkautomation
                          Copy link

                          openapi-sdkautomation bot commented Mar 18, 2020

                          azure-cli-extensions - 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

                          @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).

                          @mmyyrroonn mmyyrroonn added the ARM-overdue ARM review has not occurred within the expected timeframe label Mar 19, 2020
                          @@ -797,6 +800,14 @@
                          "definitions": {
                          "ContainerProperties": {
                          "properties": {
                          "defaultEncryptionScope": {
                          Copy link
                          Member

                          Choose a reason for hiding this comment

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

                          Adding new writable properties to a stable API is considered a breaking change. Could this instead be moved into a new API version?

                          Copy link
                          Member

                          Choose a reason for hiding this comment

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

                          Add these 2 new properties should no be breaking since:

                          1. Old sdk still work, as the new properties are optional
                          2. Old SDK won't change the properties added with new SDK

                          @anthony-c-martin anthony-c-martin added ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review potential-sdk-breaking-change 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 ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review potential-sdk-breaking-change labels Mar 20, 2020
                          @PhoenixHe-NV
                          Copy link
                          Contributor

                          /azp run

                          @azure-pipelines
                          Copy link

                          Azure Pipelines successfully started running 3 pipeline(s).

                          @qianwens qianwens added Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374 and removed Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374 labels Mar 25, 2020
                          @qianwens
                          Copy link
                          Member

                          @tonykjose , the CI failed because the breaking change in the API. Could you please help provide justified information for this breaking change and why not add a new API version?

                          @qianwens qianwens added the Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374 label Mar 25, 2020
                          @qianwens
                          Copy link
                          Member

                          @akning-ms could you please help force merge this PR since the breaking change is accepted to SDK?

                          @blueww
                          Copy link
                          Member

                          blueww commented Mar 26, 2020

                          Actually, the rest API itself is not breaking (Just add 2 properties), but the SDK generated from the swagger will break.
                          For .net SDK , this is because with autogenerated code, the new added parameter is added before the original parameter.
                          This will make the code write with before SDK build fail. But the fix is very easy.

                          I don’t think this break should block the swagger PR merge.
                          For .net SDK, I will fix the unit test after the swagger is in.
                          I think other SDK owner should also fix related Unit test. (They are owned by Dev div.)

                          Origin:
                          public static BlobContainer Create(this IBlobContainersOperations operations, string resourceGroupName, string accountName, string containerName, PublicAccess? publicAccess = default(PublicAccess?), IDictionary<string, string> metadata = default(IDictionary<string, string>))

                          New:
                          public static BlobContainer Create(this IBlobContainersOperations operations, string resourceGroupName, string accountName, string containerName, string defaultEncryptionScope = default(string), bool? denyEncryptionScopeOverride = default(bool?), PublicAccess? publicAccess = default(PublicAccess?), IDictionary<string, string> metadata = default(IDictionary<string, string>))

                          @akning-ms akning-ms merged commit 9fb516a into Azure:master Mar 26, 2020
                          00Kai0 pushed a commit to 00Kai0/azure-rest-api-specs that referenced this pull request Oct 12, 2020
                          …rride for blob container APIs. (Azure#8595)
                          
                          * Add support for default encryption scope & block encryption scope override for blob container APIs.
                          
                          * Add examples.
                          
                          * Fix prettier issues.
                          
                          * Fix prettier errors.
                          
                          * Really fix prettier errors.
                          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 ARM-overdue ARM review has not occurred within the expected timeframe 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.

                          8 participants