-
Notifications
You must be signed in to change notification settings - Fork 19
Adding the SDK Binding Support for Storage Blob #341
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
base: v4.x
Are you sure you want to change the base?
Conversation
src/deferred-binding/storage-blob/azureStorageBlobClientFactory.ts
Outdated
Show resolved
Hide resolved
src/deferred-binding/storage-blob/azureStorageBlobClientFactory.ts
Outdated
Show resolved
Hide resolved
src/deferred-binding/storage-blob/managedIdentityUserStartegy.ts
Outdated
Show resolved
Hide resolved
export function isUserBasedManagedIdentity(connectionName: string): boolean { | ||
return ( | ||
process.env[`${connectionName}__clientId`] !== undefined && | ||
process.env[`${connectionName}__credential`] !== undefined && |
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.
Ensure, process.env[${connectionName}__credential
] value is "managedIdentity"
"type": "node", | ||
"request": "launch", | ||
"program": "${workspaceRoot}/node_modules/mocha/bin/_mocha", | ||
"args": ["-r", "ts-node/register", "${relativeFile}"], |
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.
Why do we need TS test files?
If you add it here it will be used for JS too..
Also use --require like before if this is still needed
@@ -0,0 +1,90 @@ | |||
// Copyright (c) .NET Foundation. All rights reserved. |
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.
We should move this file inside storage blob? Each extension should gets its own connection details. There is no common method that can be shared with all extensions.,
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.
This is the modelBindingData( deferred binding) that we are processing, I would think for all the types we will need a way to parse the data in buffer object.
Based on the model parameter for other deferred binding types, we may have to change the logic slightly. I will refactor when, I will see the data from service bus or cosmos.
But today I will keep it unchanged
*/ | ||
constructor( | ||
strategy: StorageBlobServiceClientStrategy, | ||
containerName?: string, |
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.
This allows for null values to be passed. This code wont return any client if any of these values are null. Either add a null check or we could remove the '?'.
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.
Yeah, good point. I will change in extensions repo (as code is moved there)
This pull request introduces support for SDK binding for Azure Storage Blob in the Azure Functions Node.js library. Key changes include adding a new boolean property, deferredBinding, which allows users to enable deferred binding for storage blobs. When deferredBinding is enabled, the StorageBlobClient is returned to the user, which contains both the blobClient and containerClient, providing enhanced functionality for interacting with blob storage.
Example
Definition of StorageBlobClient