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

initial 100 service mgmt APIs generated by AutoRust #31

Merged
merged 50 commits into from Oct 20, 2020

Conversation

ctaggart
Copy link
Contributor

@ctaggart ctaggart commented Oct 12, 2020

The code generated from AutoRust is now usable. I wanted to get feedback on the generated code and how it may be integrated here. This code is generated by running cargo run --example storage_mgmt, followed by a cargo fmt in this project.

See the example storage_account_list.rs for an example of using the generated API. Example output:

# of storage accounts 1
Some("/subscriptions/54771b03-185e-4fba-98db-4324215b1031/resourceGroups/dev/providers/Microsoft.Storage/storageAccounts/ctaggart01")

Outstanding issues

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.

Overall, it's looking good. I'm really just not so sure we want this to be a separate crate. Can we talk through the pros and cons of having this as a separate crate?

rest/mgmt_storage/2019-06-01/Cargo.toml Outdated Show resolved Hide resolved
rest/mgmt_storage/2019-06-01/src/client.rs Outdated Show resolved Hide resolved
@ctaggart
Copy link
Contributor Author

Overall, it's looking good. I'm really just not so sure we want this to be a separate crate. Can we talk through the pros and cons of having this as a separate crate?

Of course! Thanks for the review. The other SDKs publish the AutoRest generated bindings in separate packages. They do keep the models and operations separate. Both group operations based on a naming convention for the operationId.

Azure SDK for Python

Azure SDK for Go

The Go SDK versions the generated Service APIs & SDKs separately. I think our code layout should look like Go, where the services are in a separate directory.

To be like the other Azure SDKs, we should publish the generated Service API crates and keep them separate from the SDK crates.

@ctaggart
Copy link
Contributor Author

ctaggart commented Oct 15, 2020

Commit 563b786 allows you to list virtual machines, but unfortunately de-serialization is failing for me.

Camerons-MacBook-Pro:azure-sdk-for-rust cameron$ cargo run --example vm_list
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/vm_list`
Error: reqwest::Error { kind: Decode, source: Error("missing field `additionalUnattendContent`", line: 1, column: 1133) }

To troubleshoot, the http response can be captured using an http proxy like mitmproxy. Then it matter of troubleshooting the serde de-serialization by reducing the json.

@ctaggart ctaggart mentioned this pull request Oct 15, 2020
@ctaggart ctaggart changed the title requesting feedback for AutoRust generated APIs initial service APIs generated by AutoRust Oct 16, 2020
@ctaggart
Copy link
Contributor Author

079dde9 added code generated for the control plane (resource manager) APIs of 70 services. A good way to review this pull request is to check out the branch. With the GitHub CLI https://cli.github.com/ installed, you can do a gh pr checkout 31. The feature names now are the tag names in the readme.md files. The default feature is the first one without preview in its name.

@rylev
Copy link
Contributor

rylev commented Oct 19, 2020

@ctaggart with the massive amount of crates in the workspace, how are clean build times affected?

@ctaggart
Copy link
Contributor Author

ctaggart commented Oct 19, 2020

The latest commit from master shows 7m 53s and the latest commit on this branch shows 14m 40s. On my laptop, not nearly that long.

@rylev
Copy link
Contributor

rylev commented Oct 19, 2020

The latest commit from master shows 7m 53s and the latest commit on this branch shows 14m 40s. On my laptop, not nearly that long.

Given the amount of code we're adding, doubling the CI time doesn't seem necessarily like a blocker, but this is something we'll want to keep an eye out for.

@ctaggart
Copy link
Contributor Author

There are 168 service directories in https://github.com/Azure/azure-rest-api-specs/tree/master/specification. If we estimate that we are able to add 70 more mgmt crates, and may be 70 svc crates, that may be another 14 minutes or 28 minutes total on GitHub Action builds.

@ctaggart
Copy link
Contributor Author

On my personal MacBook that is a couple of years old, a cargo build after a cargo clean takes just over 3 minutes. I ran it twice:

real 3m9.370s
user 18m37.587s
sys 1m18.459s

real 3m18.807s
user 19m41.191s
sys 1m23.599s

@ctaggart
Copy link
Contributor Author

ctaggart commented Oct 20, 2020

Basic error handling of errors and invalid responses is in place. Here is one that I just received:

Camerons-MacBook-Pro:azure-sdk-for-rust cameron$ cargo run --example vm_list
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target/debug/examples/vm_list`
Error: UnexpectedResponse { status_code: 404, body: b"{\"error\":{\"code\":\"InvalidResourceType\",\"message\":\"The resource type 'virtualMachines' could not be found in the namespace 'Microsoft.Compute' for api version '2020-06-30'. The supported api-versions are '2015-05-01-preview,2015-06-15,2016-03-30,2016-04-30-preview,2016-08-30,2017-03-30,2017-12-01,2018-04-01,2018-06-01,2018-10-01,2019-03-01,2019-07-01,2019-12-01,2020-06-01'.\"}}" }

@ctaggart ctaggart changed the title initial service APIs generated by AutoRust initial 100 service mgmt APIs generated by AutoRust Oct 20, 2020
@ctaggart ctaggart marked this pull request as ready for review October 20, 2020 04:08
@ctaggart
Copy link
Contributor Author

I think it is ready to merge with error responses and error handling in place. I opened up issues for the additional work.

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.

I'd like to get this into the main branch and then go from there 👍

@ctaggart ctaggart merged commit 46cc082 into Azure:master Oct 20, 2020
@ctaggart ctaggart deleted the rest branch October 20, 2020 17:55
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.

None yet

2 participants