-
Notifications
You must be signed in to change notification settings - Fork 4
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
Azdevify chat-with-your-data-solution-accelerator. #2
Conversation
@jongio - Updated according to your comments, please re-review it, thanks! |
@jongio - Updated according to your comments, please re-review this pr. Thanks! |
You don't need the full URL, |
FYI - You don't need ".git" in the name |
infra/app/adminweb.bicep
Outdated
AZURE_OPENAI_KEY: listKeys('Microsoft.CognitiveServices/accounts/${azureOpenAIName}', '2023-05-01').key1 | ||
AZURE_SEARCH_KEY: listAdminKeys('Microsoft.Search/searchServices/${azureCognitiveSearchName}', '2021-04-01-preview').primaryKey |
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 really shouldn't be using KEYS and instead use RBAC like we do in all the other samples.
Example:
You assign the right roles to the service principal of the azure resources that need to access those services and don't use KEYs in app settings at all.
- Add a parameter to the main.bicep that allows the user to either use RBAC or KEYS. Call it AUTH_TYPE and default it to rbac. And set @Allowed to 'rbac' or 'keys'
- If the auth type is rbac, then use roles. If auth type is keys, then use keys.
Use this as example for how to use RBAC https://github.com/Azure-Samples/azure-search-openai-demo/blob/main/infra/main.bicep#L353
For 'keys'
You can look up the keys with this listKeys method. Example for storage.
listKeys(resourceId(subscription().subscriptionId, rgName, 'Microsoft.Storage/storageAccounts', storageAccountName), '2021-09-01').keys[0].value
It's like what you had before, but you also include the subscriptionId and RgName
add keyvault option to get secret keys remove invalid changes add param useKeyVault
@zedy-wj Please LMK when ready for re-review. Thanks |
@zedy-wj - Please move forward to using the search SDK instead of REST calls. |
@jongio - We have updated all the code based on your comments, and updated infra/core to be consistent with azure-dev, please re-review it, thanks! |
.devcontainer/devcontainer.json
Outdated
@@ -0,0 +1,31 @@ | |||
{ |
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.
Sorry for the back and forth on this. Let's do this:
Default to using rbac.
If AUTH_TYPE is rbac, then assume the developer wants to also use key vault. Put the keys in keyvault and then read them from keyvault.
If the user sets AUTH_TYPE to keys, then read the keys from the environment and set the keys using listKeys function.
I think that will be cleaner.
@jongio - Updated according to your comments. For some things that haven’t been changed yet, please see my reply to your comments above, thanks! |
@zedy-wj - Are you ready for me to rereview this? |
@jongio - Yes, it's ready for re-review. |
@zedy-wj @v-xuto There have been a significant amount of changes to the main repo: https://github.com/Azure-Samples/chat-with-your-data-solution-accelerator, since we started this PR. They already implemented the RBAC / Keys solution in a different way. (SOrry, I didn't know this was happening). I pulled in the latest from upstream/main to jongio/main so you can see the difference. Can you please do the analysis to figure out how we can merge the two and what the differences are in implementation? |
@zedy-wj - After the merge does the app now work? |
@jongio - The merged code works well during local deployment. Since the directory structure of the project has also changed, we are solving the deployment structure problem in the |
@jongio - We've updated the code and it's working fine so far, please re-review it, thanks! |
@jongio - We removed hooks and replaced them with structural changes in the |
Thanks, I just updated main with latest from upstream, so there are merge conflicts now. Can you give me an itemized list of the tests that you ran? Like
|
@jongio - We resolved the conflict, and the update in
At present, we have the following three tests in the list, and different parameters need to be configured:
Note: The current default values are: |
This pr is all changes to convert chat-with-your-data-solution-accelerator application into an Azd template.
You can currently deploy this application using
azd
by following the steps below:azd init -t zedy-wj/chat-with-your-data-solution-accelerator-azdevify -b azdevify
azd up
@jongio - for notification.