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

[Storage] Matching _shared folder contents across all packages #28031

Merged

Conversation

vincenttran-msft
Copy link
Member

@vincenttran-msft vincenttran-msft commented Dec 21, 2022

This PR aims to unify all the contents of the _shared folders for the blob, datalake, queue, and file-share packages.

In general, my process was to use blob's _shared folder as the "source of truth" for most things as it is where most new changes will be added (and then not uniformly added to the other _shared folders)

Things to callout when unifying:

  • Biggest change came from: Support for Oauth import/export managed disks here. I am assuming this will go everywhere eventually so most of the work was getting the shared folders to all contain these changes (please correct me if this won't go everywhere, then I will have to make all the packages look similar to each other excluding this change 😢)
  • shared_access_signature.py will be different for file-share and queue since they do not support encryption scope
  • parser.py is mostly barren except for in blob -- I opted to copy blob's parser.py to every other file (which is likely overkill) just because I anticipate most new features in blob that require these parser changes will eventually trickle down to other packages
  • In response_handlers.py metadata parsing went from response.headers.items to response.http_reponse.headers.items. Not sure how this will play out because I'm assuming if it works in its current state, will the same information be contained in response.http_response vs response.headers? Pending pipeline run for the answer 😄
  • In policies.py specifically the on_request method, the queue package is the only file with this workaround (and thus will differ from other files):
# Hack to fix generated code adding '/messages' after SAS parameters
        includes_messages = request.http_request.url.endswith('/messages')
        if includes_messages:
            request.http_request.url = request.http_request.url[:-(len('/messages'))]
            request.http_request.url = urljoin(request.http_request.url, 'messages')

Therefore, the only differences in the _shared folders now is:

  • Queue generated code hack
  • shared_access_signature.py differences for packages that don't support encryption scope (file-share & queue)
  • Docstring differences that callout the package name

@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label Dec 21, 2022
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Member

@jalauzon-msft jalauzon-msft left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for working through this!

@vincenttran-msft vincenttran-msft merged commit ef02d40 into Azure:main Jan 6, 2023
@vincenttran-msft vincenttran-msft deleted the vincenttran/matching_shared branch January 6, 2023 21:55
iscai-msft added a commit that referenced this pull request Apr 26, 2023
…into version_tolerant_models

* 'main' of https://github.com/Azure/azure-sdk-for-python: (1604 commits)
  [Storage] Matching `_shared` folder contents across all packages (#28031)
  [ml] Rename MLException back to MlException (#28210)
  [EventHubs] update readme to include uamqp backcompat section (#28198)
  [Monitor][Ingestion] Adjust upload error handling (#27556)
  Add 3.11 to QA (#28169)
  p1 component deployment exisitng job definition (#28194)
  Typing for identity (#28159)
  [ServiceBus] Allow clearing of session state (#27726)
  [ML][Pipelines] Fix parallel-for validation rule: allow empty dict as loop config item (#28178)
  [ML][Pipelines] feat: enable concurrent component registration (#28118)
  [ML][Pipelines] Test: update invalid reuse test (#28180)
  [AutoRelease] t2-loadtesting-2022-12-23-48532(can only be merged by SDK owner) (#28042)
  [ML][Pipelines] Test: update unstable tests in ci (#28157)
  [ML]: Adding experimental decorator to JobDefinition (#28200)
  [Identity] Fix 'az' existence check in CliCred (#27991)
  [EventHubs] fix tracing bug (#28149)
  ShortForMigrateFrom (#28192)
  Address the API review comments (#28188)
  [SchemaRegistryAvro] update recordings (#28158)
  [EventHub] update uamqp_transport docstring (#28186)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants