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

[rest] have pipelines switch over to rest #20415

Closed
wants to merge 63 commits into from

Conversation

iscai-msft
Copy link
Contributor

fixes #19788

…into have_pipelines_support_rest

* 'main' of https://github.com/Azure/azure-sdk-for-python: (108 commits)
  Enable API review approval check for Java spring packages (Azure#20311)
  [AutoRelease] t2-iothubprovisioningservices-2021-07-15-81882 (Azure#19816)
  Increment package version after release of azure-eventgrid (Azure#20204)
  Sync eng/common directory with azure-sdk-tools for PR 1909 (Azure#20298)
  Rename attrs is metrics (Azure#20236)
  [EventHubs] checkpointstoretable - skip tests until env vars configured (Azure#20289)
  Update to use timespan (Azure#20233)
  Prevent wildcard expansion in git sparse checkout add (Azure#20267)
  Fix typo and polish the key concepts (Azure#18407)
  Fix IOT Device Update readme issue (Azure#18752)
  Add context manager API to azure.identity credentials (Azure#19746)
  Add support for 'files' configuration (Azure#20272)
  Update main for 30-close.py (Azure#20287)
  [AutoRelease] t2-purview-2021-08-13-30358 (Azure#20260)
  add implementation for checkpointstoretable (Azure#19905)
  Fix resource clean-up script (Azure#20273)
  [Tables] Fix bug in update mode (Azure#20264)
  Add Rest Method checks to Prepare-Release (Azure#20275)
  Update release date (Azure#20270)
  Update the release date for ACS chat 1.1.0b1 (Azure#20277)
  ...
…e-sdk-for-python into have_pipelines_support_rest

* 'test_to_function' of https://github.com/iscai-msft/azure-sdk-for-python:
  update changelog
  remove redundant encoding code
  remove internal_response docstrings
  add text and encoding tests
  switch text from property to method and update existing tests
…e-sdk-for-python into have_pipelines_support_rest

* 'test_to_function' of https://github.com/iscai-msft/azure-sdk-for-python:
  remove unnecessary if else statement
  remove unnecessary content setting
  convert to unicode str
  unify text and encoding accross transports
…into header_tuples

* 'main' of https://github.com/Azure/azure-sdk-for-python: (104 commits)
  [Key Vault] Add 7.3-preview support for administration (Azure#20364)
  Fix Monitor opentelemetry exporter readme issues (Azure#19038)
  More Renaming in query (Azure#20303)
  Update CODEOWNERS (Azure#20366)
  [ServiceBus] update migration guide with message count info (Azure#20360)
  [rest] change text from a property to a method (Azure#20290)
  Handle value types for results (Azure#20358)
  Remove old unused update changelog script (Azure#20357)
  bump node version (Azure#20353)
  [AutoRelease] t2-web-2021-08-03-73015 (Azure#20053)
  Fix query batch processing (Azure#20345)
  Sync eng/common directory with azure-sdk-tools for PR 1912 (Azure#20340)
  Increment version for schemaregistry releases (Azure#20326)
  [SchemaRegistry] prepare avro for release (Azure#20321)
  Update CHANGELOG.md (Azure#20334)
  Update main.py (Azure#20332)
  Removing C:\Git\azure-sdk-tools\eng\common\scripts\FilterPoliCheckResults.ps1 as it was no longer used (Azure#20325)
  Suppress CredScan warning for test proxy devcert (Azure#20324)
  [SchemaRegistry] update samples readme (Azure#20323)
  [Test Proxy] Add RecordedByProxy decorator and AzureRecordedTestCase (Azure#20138)
  ...
…into have_pipelines_support_rest

* 'main' of https://github.com/Azure/azure-sdk-for-python: (27 commits)
  Document Pod Identity's special use of client_id (Azure#20377)
  [rest] use azure json encoder for json input bodies (Azure#20361)
  CertificateCredential supports PKCS12 certs (Azure#16384)
  Bug fix for pipeline downloading incorrect package ver (Azure#20294)
  Update metadata values (Azure#20365)
  [Key Vault] Add 7.3-preview support for administration (Azure#20364)
  Fix Monitor opentelemetry exporter readme issues (Azure#19038)
  More Renaming in query (Azure#20303)
  Update CODEOWNERS (Azure#20366)
  [ServiceBus] update migration guide with message count info (Azure#20360)
  [rest] change text from a property to a method (Azure#20290)
  Handle value types for results (Azure#20358)
  Remove old unused update changelog script (Azure#20357)
  bump node version (Azure#20353)
  [AutoRelease] t2-web-2021-08-03-73015 (Azure#20053)
  Fix query batch processing (Azure#20345)
  Sync eng/common directory with azure-sdk-tools for PR 1912 (Azure#20340)
  Increment version for schemaregistry releases (Azure#20326)
  [SchemaRegistry] prepare avro for release (Azure#20321)
  Update CHANGELOG.md (Azure#20334)
  ...
…r-python into have_pipelines_support_rest

* 'header_tuples' of https://github.com/Azure/azure-sdk-for-python:
  make 2.7 compatible
  temp
  fix aiohttp kyes and values to be mutable
  temp
  change default get behavior and add tests to sync test suite
  address PR comments
  remove My from class extension names
  add support for aiohttp and comma separated values
  add tests
self._files = None

def set_json_body(self, data):
if data is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

document these as depcrecated, and try to hide with __getattr__ so they don't show up in intellisense

Copy link
Contributor Author

@iscai-msft iscai-msft Aug 25, 2021

Choose a reason for hiding this comment

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

also mention these will be removed in a future release. include in the docstrings

@@ -69,3 +44,11 @@ def get_internal_response(response):
return response._internal_response # pylint: disable=protected-access
except AttributeError:
return response.internal_response

def read_in_response(response):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe read_and_close?

@xiangyan99
Copy link
Member

great question @xiangyan99

Basically the whole point of this PR is for backcompat reasons. the azure.core.rest and azure.core.pipeline.transport requests and responses have pretty different shapes, and we want the pipelines to fully support azure.core.rest as well. After talking a lot, we decided the cleanest way to support both of these requests and responses was to give azure.core.rest the same shape as the pipeline tranpsort requests and responses (but to give it the same shape secretly, and not document it).

And the reason for having both is that: we kind of only want azure.core.rest, but for backcompat reasons, we have to keep azure.core.pipeline.transport requests and responses. We like the azure.core.rest namespace better as well, it's more user friendly

But this will be very confusing to customers that both of them have same shape but under different namespace.

If I am a customer, how can I know which one to use?

@check-enforcer
Copy link

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run python - [service] - ci

@iscai-msft
Copy link
Contributor Author

@xiangyan99 so azure.core.pipeline.transport's requests and responses were what @johanste termed "prevlic", i.e., they're technically public, but we don't expect users to use them. They're not super user friendly and you have to go pretty deep to import them. We also never really publicized users using our requests and responses. When doing LLC, we realized that those old requests and responses were not what we wanted users to use, so we created azure.core.rest.

As to your point about these rquests and responses having the same surface area confusing users over which ones to use,
a. we don't really expect users to have been manually creating and dealing with the old pipeline transport requests and responses, and
b. we're hiding the parts of the surface area that we added to rest to make it compatible with pipeline transport. If you see, we're using __getattr__ and __setattr__ magic so users won't see it documented in sphinx and intellisense

@iscai-msft iscai-msft marked this pull request as draft August 27, 2021 15:04
…ure-sdk-for-python into switch_to_rest

* 'switch_to_protocol' of https://github.com/iscai-msft/azure-sdk-for-python:
  switch from protocol to abc
  add initial tests
  update changelog
  switch to protocol
  ensure test principal creation succeeds properly (Azure#20446)
  Update question-answering readme links (Azure#20439)
  Stip empty changelog sections before release (Azure#20437)
  [AutoRelease] t2-healthcareapis-2021-08-26-27542 (Azure#20422)
  [AutoRelease] t2-iothub-2021-08-25-25696 (Azure#20409)
  Get rid of LogsBatchQueryResult (Azure#20418)
@iscai-msft iscai-msft closed this Sep 7, 2021
@iscai-msft iscai-msft deleted the switch_to_rest branch September 7, 2021 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rest] give azure.core.rest the same surface area as old reqs / responses
2 participants