Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: remove explicit UseCosmosDbPersistentStorage UseTranscriptLoggerMiddleware settings and use API to get tenant ID#2964

Merged
cwhitten merged 19 commits into
masterfrom
qika/fixcosmos
May 13, 2020
Merged

fix: remove explicit UseCosmosDbPersistentStorage UseTranscriptLoggerMiddleware settings and use API to get tenant ID#2964
cwhitten merged 19 commits into
masterfrom
qika/fixcosmos

Conversation

@zidaneymar

@zidaneymar zidaneymar commented May 9, 2020

Copy link
Copy Markdown
Contributor

Description

  1. Remove explicit UseCosmosDbPersistentStorage UseTranscriptLoggerMiddleware settings
  2. Add more error details to provision script
  3. Add guid identifier to resources (referring to OA Solution)
  4. Add tenant id retrieving in provision script.

Task Item

fixes #2927
fixes #2858

Screenshots

Have more details about the failed provision
image

@github-actions

github-actions Bot commented May 9, 2020

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 0.0% when pulling 4d509f9 on qika/fixcosmos into 4c69132 on master.

Comment thread runtime/dotnet/azurefunctions/Startup.cs Outdated

@carlosscastro carlosscastro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great! One small comment: Since you deprecated a few of the settings, it would be great to remove them from everywhere. They are still referenced in the appsettings.json file in both azure functions and azure web app runtimes. Example here. I'll set myself as waiting just to track this pending item. If there is a reason to keep them that I don't know of, all good -- just let me know :)

Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts Outdated
Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts Outdated
Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts
Comment thread Composer/plugins/samples/assets/shared/scripts/provisionComposer.js Outdated
Comment thread Composer/plugins/samples/assets/shared/scripts/provisionComposer.js
Comment thread Composer/plugins/samples/assets/shared/scripts/provisionComposer.js Outdated
Comment thread Composer/plugins/samples/assets/shared/scripts/provisionComposer.js
Comment thread runtime/dotnet/azurefunctions/Startup.cs
Comment thread runtime/dotnet/azurewebapp/Startup.cs Outdated
Comment thread runtime/dotnet/azurefunctions/appsettings.json
@carlosscastro

carlosscastro commented May 9, 2020

Copy link
Copy Markdown
Member

Thanks for the update, it's looking great. Most of my comments are minor, the only one that I feel is very relevant is that we need to somehow allow users to reach the error information when retrieving the tenant, otherwise this will be extremely hard to troubleshoot when things go wrong.

If there is some technical complexity around extracting this info in a nice way, we could at least try to log it somehwere or show it when people pass verbose or something. Let me know if you run into issues here and we can work it out.

@luhan2017 luhan2017 added R9-RC and removed R9-RC labels May 11, 2020
@luhan2017 luhan2017 changed the title fix: remove explicit UseCosmosDbPersistentStorage UseTranscriptLoggerMiddleware settings fix: remove explicit UseCosmosDbPersistentStorage UseTranscriptLoggerMiddleware settings and use API to get tenant ID May 11, 2020
@luhan2017

Copy link
Copy Markdown
Contributor

Thanks Qi, with this change: "Add guid identifier to resources (referring to OA Solution)" I can query the provision instance number for composer runtime in azure data explorer now:

image

Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts

@benbrown benbrown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor changes.

Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts
Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts Outdated
@zidaneymar

zidaneymar commented May 13, 2020

Copy link
Copy Markdown
Contributor Author

@benbrown Hi Ben, the issue about the json array empty error handling has been fixed. Could you please help review again, thanks!

carlosscastro
carlosscastro previously approved these changes May 13, 2020
Comment thread Composer/packages/lib/bot-deploy/src/botProjectDeploy.ts Outdated
Comment thread Composer/plugins/samples/assets/shared/scripts/provisionComposer.js Outdated
Co-authored-by: Carlos Castro <carlosscastro@users.noreply.github.com>
Co-authored-by: Carlos Castro <carlosscastro@users.noreply.github.com>
@cwhitten cwhitten merged commit 652868a into master May 13, 2020
@cwhitten cwhitten deleted the qika/fixcosmos branch May 13, 2020 23:50
lei9444 added a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
…gerMiddleware` settings and use API to get tenant ID (microsoft#2964)

* remove cosmosdb feature config and transcript logger feature config

* add more error details in deployment script.

* add tenant id retrieving, add reource guid identifier

* update appsettings

* add config section validation

* add more error message and comments

* fix: update bf-lu version to 4.9.0-RC6

* add error handling for tenant api

* add error handling

* update error msg

Co-authored-by: Carlos Castro <carlosscastro@users.noreply.github.com>

* update err msg

Co-authored-by: Carlos Castro <carlosscastro@users.noreply.github.com>

Co-authored-by: Lu Han <32191031+luhan2017@users.noreply.github.com>
Co-authored-by: leilzh <leilzh@microsoft.com>
Co-authored-by: Carlos Castro <carlosscastro@users.noreply.github.com>
Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants