-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Destination Azure Blob Storage: Fix overwrite mode #52610
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// 2. We really should include the namespace in the blob name, but we currently don't... | ||
// So current behavior is that if you have `public1.users` and `public2.users`, | ||
// those files will probably conflict in some way which is undesired. | ||
if (!blob.isDeleted() && blob.getName().startsWith(configuredStream.getStream().getName() + "/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to unit test this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trying to figure that out :/ might need to write something into legacy DATs, which will be not very fun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have a look at 0886140 ? It's modeled after the existing test stuff, which is doing something funky with an azure blob emulation container?
but it fails without the contains -> startsWith change, and passes afterward
0886140
to
e0a33ca
Compare
context: https://airbytehq-team.slack.com/archives/C083T7SUR0D/p1738094057451909?thread_ts=1738087568.864109&cid=C083T7SUR0D
previously:
users
andfoo_users
)*foo_users/*
, then create a blobfoo_users/<timestamp>_0
*users/*
... including the file we just created for foo_usersThis PR (I think) fixes that behavior, so that setup for
users
only touches files that actually belong to theusers
stream.(also, add a comment, b/c this destination is doing some dumb things in general)