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_blobs: Add support for snapshot blob. #966

Merged
merged 7 commits into from
Aug 4, 2022

Conversation

gorzell
Copy link
Contributor

@gorzell gorzell commented Aug 1, 2022

Add support to BlobClient for taking snapshots of a blob. For now this
reuses the opaque type that already exists for Snapshot, but it might
be worth considering adding support for treating it as a DateTime.

Add support to `BlobClient` for taking snapshots of a blob. For now this
reuses the opaque type that already exists for `Snapshot`, but it might
be worth considering adding support for treating it as a `DateTime`.
@ghost
Copy link

ghost commented Aug 1, 2022

CLA assistant check
All CLA requirements met.

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.

Thanks! This looks good.

Would you mind changing the test to use the mocking framework? There is information here about how that's supposed to work. If the docs are clear enough, let me know and we can update them.

@gorzell
Copy link
Contributor Author

gorzell commented Aug 1, 2022

Would you mind changing the test to use the mocking framework? There is information here about how that's supposed to work. If the docs are clear enough, let me know and we can update them.

Sure thing. Is there a standard place to land those tests? Should get go in the actual src file as unit tests since they don't have any external dependencies?

It looks like https://github.com/Azure/azure-sdk-for-rust/blob/36d4d163beb4f2ba8e4fa732594307d75642caf5/sdk/data_cosmos/tests/setup.rs and the tests that use it are probably the best example to go by?

Also, what do you think about relying more on the DefaultAzureCredential in the cases where real credentials are needed, rather than a smattering of different environment variables?

@rylev
Copy link
Contributor

rylev commented Aug 2, 2022

@gorzell indeed - the cosmos crate is the one to follow. Putting them in a separate tests directory is the way to go. I don't believe DefaultAzureCredential is very full featured and so it might not offer any real benefit and would make the examples potentially slightly harder to understand.

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.

Looks good! Thanks for taking the time to add the mock testing. I pretty sure you'll need to update the json docs by running the formatting script as I noted to get CI to pass, but once that happens, we can merge this.

sdk/storage/src/core/clients/storage_client.rs Outdated Show resolved Hide resolved
}
}

pub const SNAPSHOT: HeaderName = HeaderName::from_static("x-ms-snapshot");
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this is the right place for this. I'll think about it a bit more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like most of the other const of this type were in core which was a bit extreme for something storage specific. It could go in lib, or some other central place in the storage_blobs crate for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More specifically: https://github.com/gorzell/azure-sdk-for-rust/blob/main/sdk/core/src/headers/mod.rs

I think it would be fine to move this to a headers.rs file at the top level of storage_blob.


let snapshot = blob.snapshot().into_future().await.unwrap().snapshot;

trace!("crated snapshot: {:?} of {:?}", snapshot, BLOB_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to assert something here about the snapshot. Otherwise we're only testing that the deserialization doesn't outright fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll think about this, but it is a bit tricky since you get back an "opaque" value that happens to be a timestamp. You could assert that the value is in the past, but then you wouldn't be treating it as opaque. I plan to add list_snaphots in the near future, which might be the better way to improve this test?

@rylev rylev merged commit df2deaf into Azure:main Aug 4, 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