Add other Credentials support for Azure store#811
Conversation
|
sam-ayo
had a problem deploying
to
external-testing
GitHub Actions
Error
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAzureStore now accepts either an accountKey or a TokenCredential (accountKey made optional); BlobServiceClient is created with the resolved credential. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sam-ayo
had a problem deploying
to
external-testing
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 3-7: The root package.json currently contains the `@tus/azure-store`
package manifest ("name", "version", "description", "main", "exports"); replace
it with the monorepo root manifest by restoring "private": true and
"workspaces": ["packages/*", "test"] and any other root-level fields from prior
commits, and move the package-specific fields ("name": "@tus/azure-store",
"version": "2.0.0", "description", "main", "exports") into the package manifest
for the azure-store package (packages/azure-store package.json). Ensure the root
package.json does not declare a package "name" or version for a single package
and that workspaces are present to restore the monorepo structure.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| this.storePath = storePath | ||
| this.testFilePath = path.resolve(fixturesPath, this.testFileName) | ||
| }) | ||
| describe('with Account key', () => { |
There was a problem hiding this comment.
We dont' need this redundant wrapping of describe's
| type ContainerClient, | ||
| StorageSharedKeyCredential, | ||
| } from '@azure/storage-blob' | ||
| import type {TokenCredential} from '@azure/core-auth' |
There was a problem hiding this comment.
Do we really need a new dep just for this type? Would be great if this is not needed.
There was a problem hiding this comment.
I agree in general to not need to add a new dependency only to get a type, but I believe it's necessary in this case to show that a user could use other credential authentication to connect to the storage account.
packages/azure-store/src/index.ts
Outdated
| accountKey?: string | ||
| credential?: TokenCredential |
There was a problem hiding this comment.
If it's one or the other you should make this a discriminated union
|
Taking a step back, it would be great if people can just pass their own credentials thing and we don't need to manually wrap every option. Not sure if that makes sense, haven't looked into it enough. |
sam-ayo
requested a deployment
to
external-testing
GitHub Actions
Waiting
Could you expatiate on what you mean? Not sure I understand |
2 participants
This PR includes support for using other azure credentials like DefaultAzureCredential instead of using the storage account key which like any key comes with it's security risks