Azure / azure-sdk-for-rust

This repository is for active development of the *unofficial* Azure SDK for Rust. This repository is *not* supported by the Azure SDK team.
MIT License
676 stars 231 forks source link

Fix broken code behind client_certificate feature #1650

Open akinnane opened 2 months ago

akinnane commented 2 months ago

Building the SDK with cargo --all-targets --all-features fails with two different errors.

1.

error[E0599]: no variant or associated item named `http_response_from_body` found for enum `azure_core::error::ErrorKind` in the current scope
   --> sdk/identity/src/token_credentials/client_certificate_credentials.rs:262:35
    |
262 |             return Err(ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error());
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^
    |                                   |
    |                                   variant or associated item not found in `ErrorKind`
    |                                   help: there is an associated function with a similar name: `http_response`

ErrorKind::http_response_from_body was changed to ErrorKind::http_response_from_parts in https://github.com/Azure/azure-sdk-for-rust/pull/1631 I've changed this to use the new method from that PR.

2.

error[E0599]: the method `clone` exists for enum `Result<Url, Error>`, but its trait bounds were not satisfied
   --> sdk/identity/src/token_credentials/client_certificate_credentials.rs:131:64
    |
131 |             authority_host: options.options().authority_host().clone(),
    |                                                                ^^^^^ method cannot be called on `Result<Url, Error>` due to unsatisfied trait bounds
    |
   ::: /home/a/repos/akinnane_azure_sdk_for_rust/sdk/core/src/error/mod.rs:87:1
    |
87  | pub struct Error {
    | ---------------- doesn't satisfy `azure_core::Error: Clone`
    |
   ::: /home/a/.rustup/toolchains/1.74.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:502:1
    |
502 | pub enum Result<T, E> {
    | --------------------- doesn't satisfy `Result<Url, azure_core::Error>: Clone`
    |
    = note: the following trait bounds were not satisfied:
            `azure_core::Error: Clone`
            which is required by `Result<Url, azure_core::Error>: Clone`

This was added in https://github.com/Azure/azure-sdk-for-rust/commit/b28ca1d05cbc226e2efef8109b3544954fbe3d37 I've changed the return type to be be an Azure::Result and used ? to get Url from .authority_host()

3.

error[E0463]: can't find crate for `log`
 --> sdk/storage_blobs/tests/blob.rs:3:1
  |
3 | extern crate log;
  | ^^^^^^^^^^^^^^^^^ can't find crate

error: cannot find macro `trace` in this scope
   --> sdk/storage_blobs/tests/blob.rs:267:5
    |
267 |     trace!("running list_containers");
    |     ^^^^^
    |
help: consider importing one of these items
    |
5   + use tracing::log::trace;
    |
5   + use tracing::trace;
    |

error: cannot find macro `trace` in this scope
   --> sdk/storage_blobs/tests/blob.rs:276:9
    |
276 |         trace!("ret {:?}\n\n", ret);
    |         ^^^^^
    |
help: consider importing one of these items
    |
5   + use tracing::log::trace;
    |
5   + use tracing::trace;
    |

I've added tracing to the dev-dependencies for consistency with the other SDK crates.

There were a bunch of warning emmitted after fixing those and I've fixed those too.

I've also added extra steps to eng/scripts/sdk_tests.sh to run check and test with --all-targets --all-features to prevent this happening again, but you might want to do this differently on GitHub Actions.

akinnane commented 2 months ago

@microsoft-github-policy-service agree

gurkanindibay commented 2 months ago

@akinnane could you rerun the failed tests so that PR will be merged after successful execution

akinnane commented 2 months ago

@akinnane could you rerun the failed tests so that PR will be merged after successful execution

There are two failing tests.

Autorust tests

Looks like a new lint was added in 1.78.0 which was released a few hours ago

https://blog.rust-lang.org/2024/05/02/Rust-1.78.0.html https://rust-lang.github.io/rust-clippy/master/index.html#/to_string_trait_impl

 error: direct implementation of `ToString`
   --> codegen/src/codegen.rs:346:1
    |
346 | / impl ToString for TypeNameCode {
347 | |     fn to_string(&self) -> String {
348 | |         self.to_type().into_token_stream().to_string()
349 | |     }
350 | | }
    | |_^
    |
    = help: prefer implementing `Display` instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#to_string_trait_impl
    = note: `-D clippy::to-string-trait-impl` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::to_string_trait_impl)]`

error: could not compile `autorust_codegen` (lib) due to 1 previous error
Error: Process completed with exit code 101.

SDK Tests

The tests are looking for a COSMOS_ACCOUNT id that will need to be supplied to the to the Github Actions


     Running tests/attachment_operations.rs (target/debug/deps/attachment_operations-8317b681f0a86835)

running 1 test
test attachment_operations ... FAILED

failures:

---- attachment_operations stdout ----
thread 'attachment_operations' panicked at sdk/data_cosmos/tests/setup.rs:13:37:
Set env variable COSMOS_ACCOUNT first!: NotPresent
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    attachment_operations

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `-p azure_data_cosmos --test attachment_operations`
Error: Process completed with exit code 101.
alex403in commented 1 month ago

@heaths Please could you add the COSMOS_ACCOUNT and related variables to Github Secrets so the tests can find them?

heaths commented 1 month ago

@heaths Please could you add the COSMOS_ACCOUNT and related variables to Github Secrets so the tests can find them?

We do not have provisioned resources for this unofficial project. Even for our official SDKs we provision as-needed, which is not set up for this repo just yet...but changes are coming.

Or are you suggesting it should be set to some demo, non-secret account? I'm not aware of any, though I know some other services have that e.g., Storage, IIRC.

bmorton commented 1 month ago

We're using this forked code and it's working for us right now, but I'm worried about the path forward to getting this merged. Does this need the COSMOS_ACCOUNT for something that it's changing specifically? Are other PRs blocked on this same issue?

heaths commented 1 month ago

One small request, and please rebase. We can get this in, then.