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.

Sign up for GitHub

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

Jump to bottom

Support listing blobs recursively in azure-blob scaler #2036

Closed
wants to merge 2 commits into from
Closed

Support listing blobs recursively in azure-blob scaler #2036

wants to merge 2 commits into from

Conversation

ahmelsayed
Copy link
Contributor

@ahmelsayed ahmelsayed commented Aug 16, 2021 •
edited by tomkerkhove
Loading

Checklist

Fixes #1789

@ahmelsayed ahmelsayed requested a review from zroubalik as a code owner August 16, 2021 18:45
@ahmelsayed ahmelsayed mentioned this pull request Aug 16, 2021
Closed
ahmelsayed
ahmelsayed commented Aug 16, 2021
View reviewed changes
pkg/scalers/azure_blob_scaler.go Outdated
Comment on lines 89 to 91
if !strings.HasSuffix(meta.blobPrefix, defaultBlobDelimiter) {
meta.blobPrefix += defaultBlobPrefix
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned on the issue #1789

One complication: it seems that the scaler was appending blobDelimiter to blobPrefix, which isn't right. To avoid having a breaking change, I append the default blobDelimiter, i.e: /, to the blobPrefix if it's not there. But we might consider removing that in the future. It'll be a breaking change though, so we might only do it for v3?

@ahmelsayed
fix typo
7681d84 
Signed-off-by: Ahmed ElSayed <[email protected]>
@tomkerkhove
Copy link
Member

Thanks for doing this @ahmelsayed!

I've added a comment on the docs PR, though, as this feels like a separate flag on the scaler: kedacore/keda-docs#512 (comment)

zroubalik
zroubalik approved these changes Aug 17, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, but as I mentioned on the issue, I wouldn't be afraid to introduce a breaking change in the next release, if you think that it is the right solution (with proper documentation).

It's up to you to decide 🤷‍♂️

@zroubalik zroubalik added this to the v2.5.0 milestone Aug 18, 2021
@ahmelsayed ahmelsayed marked this pull request as draft August 18, 2021 23:30
@tomkerkhove
Copy link
Member

Any update on this?

@tomkerkhove
Copy link
Member

@ahmelsayed Any update on this?

@tomkerkhove tomkerkhove modified the milestones: v2.5.0, v2.6.0 Nov 23, 2021
@tomkerkhove tomkerkhove removed this from the v2.6.0 milestone Jan 4, 2022
@tomkerkhove
Copy link
Member

Any update on this @ahmelsayed ?

@ahmelsayed ahmelsayed closed this Mar 3, 2022
@zroubalik zroubalik deleted the ahmels/1789 branch March 24, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@arschles arschles arschles approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Azure Blob Storage Scaler doesn't list blobs recursively
4 participants
@ahmelsayed @tomkerkhove @arschles @zroubalik