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.

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

fix: deployment script, decouple debugging and deployment settings #2153

Merged
merged 10 commits into from
Mar 6, 2020

Conversation

zidaneymar
Copy link
Contributor

Description

fix #2007

  1. Fix several bugs of deployment script.
  2. Decouple appsettings to appsettings.Devlopment.json(for composer) and appsettings.Production.json(for azure deployment)

Task Item

Screenshots

Use appsettings.Deployment.json for local debug, use appsettings.Production.json for azure deployment.
@boydc2014
Copy link
Contributor

boydc2014 commented Mar 4, 2020

@zidaneymar can you elaborate on the design of decoupling appsetttings.json into dev.json and prod.json? a few questions is

  1. is this just for publish or will composer need to write to a appsettings.{env}.json instead of appsettings.json?
  2. is this a breaking change? what's the impact here? at least i saw now we need a new arugment to start runtime
  3. i see you hard code development.json and prod.json, what if other env?

anyhow, let's get time to address those, and i put an onhold on this one in case accidentally merged.

@zidaneymar
Copy link
Contributor Author

@zidaneymar can you elaborate on the design of decoupling appsetttings.json into dev.json and prod.json? a few questions is

  1. is this just for publish or will composer need to write to a appsettings.{env}.json instead of appsettings.json?
  2. is this a breaking change? what's the impact here? at least i saw now we need a new arugment to start runtime
  3. i see you hard code development.json and prod.json, what if other env?

anyhow, let's get time to address those, and i put an onhold on this one in case accidentally merged.

  1. For now composer will update the customized settings file which is in ComposerDialogs\settings\appsettings.json, I think the root appsettings.json will not be changed by composer, it has many startup parameters for botruntime.
  2. This is to fix Response status code does not indicate success: 403 (Forbidden) after Azure deploym,e #2007, previously the deployment script will write the app id, app password, blob storage and other config to the root appsettings.json, if user want to start up the local debugging environment, it will get an 401 unauthorized since the runtime has appid and pwd now.
    So we need to decouple the composer dubugging env(deployment) and deployment env(production) to different files.
  3. I'm not sure about the other env, in my understanding we have only two environments for debugging(users could set it as 'composer' but it is still used for debugging) and deployment.

@@ -152,6 +152,8 @@ export class CSharpBotConnector implements IBotConnector {
`bin/Debug/${envSettings.runtimeFrameworkVersion}/BotProject.dll`,
`--urls`,
this.endpoint,
`--environment`,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to update the protocol to add a new parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need to update the protocol to add a new parameter

dotnet receive the env parameter to determine whether it's under development

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this param will be sent to 'dotnet run' iteself, not a botproject level param.
https://stackoverflow.com/questions/50821504/asp-net-core-set-hosting-environment-in-build-process

@github-actions
Copy link

github-actions bot commented Mar 6, 2020

Coverage Status

Coverage remained the same at 40.937% when pulling 3bd704b on qika/deployupdate2 into 964b8d6 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants