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

address some concerns in initial device_update PR #716

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

bmc-msft
Copy link
Contributor

@bmc-msft bmc-msft commented Apr 7, 2022

This PR adds the following changes:

  • moves to enabling the client to support light-weight clones rather than managing lifetimes for the client.
  • removes the need for the client to be mutable
  • moves to using azure_identity's AutoRefreshingTokenCredential rather than by-hand token refresh.
  • replaces runtime panics with returning errors
  • moves helper functions to return the DeserializeOwned trait, rather than strings which get immediately deserialized
  • moves many of the errors to use the azure_core error types (in prep for moving to the pipeline archtecture)
  • moves to using query_pairs_mut to add URL params, rather than building query strings by hand
  • simplfies creating the mock client used for testing

Things still to do (in follow-up PRs):

This adds the following changes:
* moves to enabling the client to support light-weight clones rather than managing lifetimes for the client.
* removes the need for the client to be mutable
* moves to using azure_identity's AutoRefreshingTokenCredential for refreshing tokens as needed.
* replaces runtime panics with returning errors
* moves helper functions to return the DeserializeOwned trait, rather than strings which get immediately deserialized
* moves many of the errors to use the azure_core error types (in prep for moving to the pipeline archtecture)
* moves to using query_pairs_mut to add URL params, rather than building query strings by hand
* simplfies creating the mock client used for testing
@bmc-msft bmc-msft requested a review from rylev April 7, 2022 01:09
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

This looks good. I think we should maybe start moving over to the pipeline architecture ASAP as we might be doing some work that will just go away once the transition is made. I, however, still own a written transition guide...

@rylev rylev merged commit 13fdcb8 into Azure:main Apr 8, 2022
@bmc-msft bmc-msft deleted the device-update-pr-comments branch April 27, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants