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

Move IoT Hub to operation builder pattern #960

Merged
merged 14 commits into from
Jul 28, 2022
Merged

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Jul 27, 2022

This changed the user interface a little bit, and made it a tiny bit worse IMO, but I think we can easily regain that usability in a follow up.

NOTE: This does not update IoT Hub to use the pipeline yet. That will be a follow-up PR.

This is part of the work for #802

@rylev rylev requested a review from bmc-msft July 27, 2022 14:24
.get_configuration(configuration_id)
.into_future()
.await?;
let configuration: Configuration = configuration.try_into()?;
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 is a bit of a shame that we are now returning an unstructured type.

@@ -139,6 +140,7 @@ macro_rules! operation {
) => {
azure_core::__private::paste! {
#[derive(Debug, Clone)]
$(#[$outer])*
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

configuration_id: String,
priority: u64,
target_condition: String,
etag: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of exposing two operations, we could do what the Python SDK does, which is to have a single create_or_update function, and provide the optional parameter if_match.

https://docs.microsoft.com/en-us/python/api/azure-mgmt-iothub/azure.mgmt.iothub.v2021_07_02.operations.iothubresourceoperations?view=azure-python#azure-mgmt-iothub-v2021-07-02-operations-iothubresourceoperations-begin-create-or-update

if_match
ETag of the IoT Hub. Do not specify for creating a brand new IoT Hub. Required to update an existing IoT Hub.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be done across the crate.

Copy link
Contributor Author

@rylev rylev Jul 28, 2022

Choose a reason for hiding this comment

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

That sounds fine. I think the entire client interface needs to be rethought through1 so maybe we leave that for a different PR?

Footnotes

  1. This SDK does not follow the "client tree" model and instead ops for one mega client. We should probably move away from that.

/// The DeleteConfigurationBuilder is used to construct a request to delete a configuration.
DeleteConfiguration,
client: ServiceClient,
if_match: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar with above, we should standardize on etag vs if_match. Additionally, this should be optional.

https://docs.microsoft.com/en-us/rest/api/iothub/service/configuration/delete#request-headers

@cataggar cataggar mentioned this pull request Jul 28, 2022
9 tasks
@bmc-msft bmc-msft merged commit 0ec7b44 into Azure:main Jul 28, 2022
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