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

[Storage] get_blob_properties for BlobClient #2014

Merged
merged 25 commits into from
Feb 26, 2025

Conversation

vincenttran-msft
Copy link
Member

@vincenttran-msft vincenttran-msft commented Jan 22, 2025

This PR contains the following:

  • Fleshed out implementation of BlobClient to support get_blob_properties
  • get_blob_properties and simple test cases (get properties with existing blob, container not exists case)

@vincenttran-msft vincenttran-msft marked this pull request as ready for review January 24, 2025 20:39
@vincenttran-msft vincenttran-msft changed the title [Storage] get_blob_properties initial implementation [Storage] get_blob_properties for BlobClient Jan 24, 2025
heaths
heaths previously requested changes Jan 27, 2025
@vincenttran-msft vincenttran-msft dismissed heaths’s stale review January 31, 2025 23:25

Will fix in upcoming PR.

@vincenttran-msft
Copy link
Member Author

Hi @heaths Heath, I have documented the remaining 4 feedback items into my notes, and will address them in the following PR. I have commented out the testcases since they currently rely on static resources and have a hard dependency on some NYI functionality. With that in mind- please let me know if there is any other showstoppers in this specific PR. Thanks!

@vincenttran-msft vincenttran-msft requested a review from a team as a code owner February 12, 2025 00:30
@vincenttran-msft
Copy link
Member Author

I was under the impression this PR was no longer passing due to merging in main and messing up the Cargo.lock, but even after attempting to merge again and cargo generate lock-file, I am still hitting some errors here. Some assistance on getting CI green alongside the review would be greatly appreciated!

@vincenttran-msft
Copy link
Member Author

Hi @heaths Heath, whenever you get a free moment if you could take a look at this to help resolve the CI issues and any merge-blocking stoppers. Thanks!

@heaths
Copy link
Member

heaths commented Feb 20, 2025

Hi @heaths Heath, whenever you get a free moment if you could take a look at this to help resolve the CI issues and any merge-blocking stoppers. Thanks!

@vincenttran-msft you need to fetch upstream/main and rebase on it. The issues you are running into were fixed in main. Don't merge, but rebase.

@vincenttran-msft vincenttran-msft force-pushed the vincenttran/get_blob_props_mvp branch from 2603d15 to 6c7c975 Compare February 21, 2025 01:37
@vincenttran-msft
Copy link
Member Author

@heaths I have addressed your comments, rebased, and resolved the Build Pack Crates issue by changing
azure_core_test.workspace = true -> azure_core_test.path = "../../core/azure_core_test"
under dev-dependencies in the Cargo.toml.

The remaining error is in the "Build Analyze" step, specifically Run source analysis:

Checking azure_security_keyvault_test v0.1.0 (/mnt/vss/_work/1/s/sdk/keyvault/azure_security_keyvault_test)
Invoke-LoggedCommand: /mnt/vss/_work/1/s/eng/scripts/Analyze-Code.ps1:33
Line |
  33 |    Invoke-LoggedCommand "cargo +$Toolchain clippy --target=wasm32-unkn …
     |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | ##[error]Command failed to execute (00:00:53.4807897): cargo +stable
     | clippy --target=wasm32-unknown-unknown --workspace --keep-going --no-deps

##[error]PowerShell exited with code '1'.
Finishing: Run source analysis

It seems to be having issues while checking azure_security_keyvault_test v0.1.0, any ideas? Thanks!

@heaths
Copy link
Member

heaths commented Feb 25, 2025

Building and testing all features in the workspace works fine on my machine. There were some changes that went in today that may fix them, though. Try rebasing again.

@heaths
Copy link
Member

heaths commented Feb 25, 2025

The command failing here works fine on my box, and I just pulled upstream/main once again:

cargo +stable clippy --target=wasm32-unknown-unknown --workspace --keep-going --no-deps

You've been merging (in general, don't do that unless you're trying to main history for some reason), and I'm guessing you have a bad merge at some point. If all your changes have been under sdk/storage, I recommend from within your topic branch:

git fetch upstream main
git rebase -X theirs upstream/main

The problem you're running into is something I believe I fixed last Friday (was merged today, Monday).

@heaths
Copy link
Member

heaths commented Feb 25, 2025

Also, I believe your merge is dirty because at one point Cargo.lock was accidentally upgraded to all new versions, which caused some issues with getrandom used by uuid. I had to downgrade uuid from 0.3 to 0.2 (what we had) because of it; lots of people are running into this problem because of a breaking change in getrandom and because we have dependencies that take a dependency on an older version: the workaround for the older and newer versions conflicts with each other.

@vincenttran-msft
Copy link
Member Author

@heaths Thanks for that callout. After merging and reverting the lockfile and doing a clean cargo build on the crate, we are now back to passing! Thanks for the help Heath, it should now be ready for a final review.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

A couple nits/tips but nothing blocking. LGTM

@vincenttran-msft vincenttran-msft merged commit 319a7c5 into main Feb 26, 2025
18 checks passed
@vincenttran-msft vincenttran-msft deleted the vincenttran/get_blob_props_mvp branch February 26, 2025 19:49
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.

4 participants