Fix broken code behind client_certificate feature

[akinnane]

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

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 #1631 I've changed this to use the new method from that PR.

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 b28ca1d
I've changed the return type to be be an Azure::Result and used ? to get Url from .authority_host()

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]

@microsoft-github-policy-service agree

[heaths]

I have a question, but otherwise LGTM.

[heaths]

Do you know which targets generated the errors? I'm surprised we hadn't run into them before, and it may be better to test those targets in parallel rather than every time this script is run. Besides, there's some major changes coming soon and these scripts will likely be removed in favor of parallel jobs. I want to make sure we cover what failed.

[akinnane]

The feature generating the error is client_certificate from src/identity. I was just using --all-features to check I wasn't breaking anything else when I was making changes. I can change change the script to just include that feature, and then you can run all features in parallel when the your major changes land.

[NickButcher1]

My repo https://dev.azure.com/msazuredev/AzureForOperators/_git/a40na-ingestion-file-uploader is also hitting this issue after updating 0.19.0 -> 0.20.0 because we use the client_certificate feature.

[heaths]

I can change change the script to just include that feature, and then you can run all features in parallel when the your major changes land.

@akinnane yes, can you make that change instead?

[gurkanindibay]

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

[akinnane]

@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]

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

[heaths]

@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]

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]

I can change change the script to just include that feature, and then you can run all features in parallel when the your major changes land.

@akinnane yes, can you make that change instead?

[heaths]

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

[gurkanindibay]

@heaths your suggestion has already been implemented. Can you merge now?

[heaths]

Odd. I rebased and fixed the conflict, but pushing closed the PR and I can't seem to reopen it.

[heaths]

Ah, because I didn't push with HEAD:main but I can't force push to your main (always branch in git; never work in main).

[heaths]

Closing in lieu of #1706

[heaths]

@heaths your suggestion has already been implemented. Can you merge now?

Actually, seems it hasn't. I'll have to fix it in my branch, though, because this remote is locked.