apache / opendal

Apache OpenDAL: One Layer, All Storage.

https://opendal.apache.org

Apache License 2.0

3.45k stars 482 forks source link

feat: Add if_none_match for write #5129

Closed ForestLH closed 1 month ago

ForestLH commented 2 months ago

Which issue does this PR close?

Closes #5097

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Yes, add an optionif_none_match for write_with.

ForestLH commented 1 month ago

It mostly looks good to me. To make this feature work, we should also add a flag in Capability called write_with_if_none_match. Additionally, we can add a behavior test based on this.

Thank you very much for your careful help. I have revised all your comments. 😄

Xuanwo commented 1 month ago

Hi, the tests failed because the version we used for testing doesn't support this feature yet. The upstream added this feature in this commit.

Would you like to upgrade MinIO to the latest version, such as RELEASE.2024-09-13T20-26-02Z, in a new PR?

ForestLH commented 1 month ago

Hi, the tests failed because the version we used for testing doesn't support this feature yet. The upstream added this feature in this commit.

Would you like to upgrade MinIO to the latest version, such as RELEASE.2024-09-13T20-26-02Z, in a new PR?

I'm sorry I didn't think of that. Thank you for your careful help. And you mean upgrade MinIO version at here? Like this PR?

tisonkun commented 1 month ago

And you mean upgrade MinIO version at here? Like this https://github.com/apache/opendal/pull/5142?

@ForestLH Yes. Please try it out.

FYI @Xuanwo reaction on comments won't send a notification so that the author may assume he/she isn't replied.

ForestLH commented 1 month ago

FYI @Xuanwo reaction on comments won't send a notification so that the author may assume he/she isn't replied.

Thanks for the tip!

ForestLH commented 1 month ago

Can you help me review the code, I have rebase the code to the latest. Because I don't have a relevant environment, and I'm still learning OpenDAL, the code I write may not be comprehensive. @Xuanwo

Xuanwo commented 1 month ago

if_none_match also supported by gcs and azblob, would you like to implement them too? cc @ForestLH

ForestLH commented 1 month ago

if_none_match also supported by gcs and azblob, would you like to implement them too? cc @ForestLH

Sure, I appreciate to be able to do this work! 😄 But I don't have gcs and azblob acounts. What better way than to sign up for gcs and azblob account? ps, Shall I open another issue about this?