-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Azure App service Manage & Deploy Task Spec changes #4125
Conversation
tl.debug('App Service State : ' + appServiceDetails.properties.state); | ||
while(!(appServiceDetails.properties.state == "Running" || appServiceDetails.properties.state == "running")) | ||
{ | ||
appServiceDetails = await azureRmUtil.getAppServiceDetails(endPoint, resourceGroupName, webAppName, specifySlotFlag, slotName); |
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.
do you want to add a wait() here and retry after certain intervals?
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.
wait is actually not required as we just poll for the state of the app and not on any operation.
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 check for hasProperty("properties") and hasProperty("state") after getting appServiceDetails in while loop
appServiceDetails = await azureRmUtil.getAppServiceDetails(endPoint, resourceGroupName, webAppName, specifySlotFlag, slotName); | ||
} | ||
} | ||
await waitForAppServiceToStart(endPoint, resourceGroupName, webAppName, specifySlotFlag, slotName); | ||
break; | ||
} | ||
case "Stop Azure App Service": { |
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.
Should we wait for Stop as well?
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.
Stop actually stops the App Service to accept any request. We should be good here.
@@ -403,7 +403,7 @@ async function getPostDeploymentScriptLogs(publishingProfile, physicalPath, uniq | |||
console.log(tl.loc('stderrFromScript')); | |||
if(scriptReturnCode != '0') { | |||
tl.error(stderrLog); | |||
throw Error(tl.loc('ScriptExecutionOnKuduFailed', scriptReturnCode, stderrLog)); |
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 this change?
@@ -475,6 +475,7 @@ | |||
"ScriptStatusTimeout": "Unable to fetch script status due to timeout.", | |||
"PollingForFileTimeOut": "Unable to fetch script status due to timeout. You can increase the timeout limit by setting 'appservicedeploy.retrytimeout' variable to number of minutes required.", | |||
"InvalidPollOption": "Invalid polling option provided: %s.", | |||
"MissingWebConfigParameters": "Some web.config parameters are missing" | |||
"MissingWebConfigParameters": "Some web.config parameters are missing", |
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.
Is there a guidance we can provide? Either a link where defaults are available... or guidance on how and where to edit.
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.
Help Text need to be updated at many places. Will update once I get the message reviewed by @azooinmyluggage
@@ -17,3 +17,53 @@ function replaceMultiple(text: string, substitutions: any): string { | |||
} | |||
return text; | |||
} | |||
|
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.
The file name as generatewebconfig is not appropriate any more. Should this be webconfigutils?
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.
Also reconcile between generateWebConfigFile and addWebConfigFile
|
||
cmd /c "kuduPostDeploymentScript_%1.cmd" > "stdout_%1.txt" 2> "stderr_%1.txt" && ( |
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.
Try the changes that we discussed.
@@ -140,7 +140,7 @@ | |||
"loc.messages.ScriptStatusTimeout": "Unable to fetch script status due to timeout.", | |||
"loc.messages.PollingForFileTimeOut": "Unable to fetch script status due to timeout. You can increase the timeout limit by setting 'appservicedeploy.retrytimeout' variable to number of minutes required.", | |||
"loc.messages.InvalidPollOption": "Invalid polling option provided: %s.", | |||
"loc.messages.MissingWebConfigParameters": "Some web.config parameters are missing", | |||
"loc.messages.MissingAppTypeWebConfigParameters": "-appType attribute is missing in Web.config parameters.", |
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.
Is apptype the only config param that can be missing? Should we also report if other required parameters are not present?
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 can override with default values. I ll update it in this PR.
@@ -23,6 +23,8 @@ | |||
"loc.input.label.Slot": "Slot", | |||
"loc.input.label.ExtensionsList": "Install Extensions", | |||
"loc.input.help.ExtensionsList": "Site Extensions run on Microsoft Azure App Service. You can install set of tools as site extension and better manage your Azure App Service. The App Service will be restarted to make sure latest changes take effect.", | |||
"loc.input.label.OutputVariable": "Output variable", | |||
"loc.input.help.OutputVariable": "Provide comma separated list of variables that saves the local path of the extension which can be used in subsequent tasks.", |
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.
"Provide the variable name for the local installation path for the selected extension.
Note: In case of multiple extensions selected for installation, provide comma separated list of variables that saves the local path for each of the selected extension in the order it appears in the Install Extension field"
@@ -475,6 +475,10 @@ | |||
"ScriptStatusTimeout": "Unable to fetch script status due to timeout.", | |||
"PollingForFileTimeOut": "Unable to fetch script status due to timeout. You can increase the timeout limit by setting 'appservicedeploy.retrytimeout' variable to number of minutes required.", | |||
"InvalidPollOption": "Invalid polling option provided: %s.", | |||
"MissingWebConfigParameters": "Some web.config parameters are missing" | |||
"MissingAppTypeWebConfigParameters": "-appType attribute is missing in Web.config parameters.", |
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.
Attribute '-appType' is missing in the Web.config parameters. Valid values for '-appType' are: 'python_Bottle', 'python_Django', 'python_Flask' and 'node'.
For example, '-appType python_Bottle' (sans-quotes) in case of Python Bottle framework.
@@ -475,6 +475,10 @@ | |||
"ScriptStatusTimeout": "Unable to fetch script status due to timeout.", | |||
"PollingForFileTimeOut": "Unable to fetch script status due to timeout. You can increase the timeout limit by setting 'appservicedeploy.retrytimeout' variable to number of minutes required.", | |||
"InvalidPollOption": "Invalid polling option provided: %s.", | |||
"MissingWebConfigParameters": "Some web.config parameters are missing" | |||
"MissingAppTypeWebConfigParameters": "-appType attribute is missing in Web.config parameters.", | |||
"AutoDetectDjangoSettingsFailed": "Unable to detect DJANGO_SETTINGS_MODULE (settings.py). Please ensure that the file exists or provide the correct path in Web.config parameters input in the following format '<folder_name>.settings'", |
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.
"Unable to detect DJANGO_SETTINGS_MODULE 'settings.py' file path. Ensure that the 'settings.py' file exists or provide the correct path in Web.config parameter input in the following format '-DJANGO_SETTINGS_MODULE <folder_name>.settings'",
"MissingWebConfigParameters": "Some web.config parameters are missing" | ||
"MissingAppTypeWebConfigParameters": "-appType attribute is missing in Web.config parameters.", | ||
"AutoDetectDjangoSettingsFailed": "Unable to detect DJANGO_SETTINGS_MODULE (settings.py). Please ensure that the file exists or provide the correct path in Web.config parameters input in the following format '<folder_name>.settings'", | ||
"FailedToApplyTransformation": "Unable to apply transformation for the given package. Please follow below steps to debug the issue. \n1. Transformation is applied for MSBuild generated packge during build time. Remove <DependentUpon> Tag for each config file. \n2. Ensure that the config file and transformation file are present in the same folder inside the package.", |
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.
"Unable to apply transformation for the given package. Verify the following. \n1. Whether the Transformation is already applied for the MSBuild generated package during build. If yes, remove the tag for each config in the csproj file and rebuild. \n2. Ensure that the config file and transformation files are present in the same folder inside the package.",
"MissingAppTypeWebConfigParameters": "-appType attribute is missing in Web.config parameters.", | ||
"AutoDetectDjangoSettingsFailed": "Unable to detect DJANGO_SETTINGS_MODULE (settings.py). Please ensure that the file exists or provide the correct path in Web.config parameters input in the following format '<folder_name>.settings'", | ||
"FailedToApplyTransformation": "Unable to apply transformation for the given package. Please follow below steps to debug the issue. \n1. Transformation is applied for MSBuild generated packge during build time. Remove <DependentUpon> Tag for each config file. \n2. Ensure that the config file and transformation file are present in the same folder inside the package.", | ||
"AutoParameterizationMessage": "ConnectionString attributes in Web.config is parameterized by default. Please be aware that transformation has no effect on connectioString attributes as the same value is overridden during deployment. You can disable the auto parameterization by setting /p:AutoParameterizationWebConfigConnectionStrings=False during MSBuild package generation.", |
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.
ConnectionString attributes in Web.config is parameterized by default. Note that the transformation has no effect on connectionString attributes as the value is overridden during deployment by 'Parameters.xml or SetParameters" files. You can disable the auto-parameterization by setting /p:AutoParameterizationWebConfigConnectionStrings=False during MSBuild package generation."
"label": "Output variable", | ||
"defaultValue": "", | ||
"visibleRule": "Action = Install Extensions", | ||
"helpMarkDown": "Provide the variable name for the local installation path for the selected extension.<br />Note: In case of multiple extensions selected for installation, provide comma separated list of variables that saves the local path for each of the selected extension in the order it appears in the Install Extension field" |
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.
Give an example. Should the name be just varName or $(varName)?
@@ -82,7 +82,7 @@ export function getChildElementsByTagName(node, tagName) { | |||
} | |||
var liveChildNodes = getChildElementsByTagName(children[i], tagName); | |||
if(liveChildNodes && liveChildNodes.length > 0){ | |||
liveNodes.concat(liveChildNodes); | |||
liveNodes = liveNodes.concat(liveChildNodes); |
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 this change?
if(!isFolderBasedDeployment && utility.isMSDeployPackage(webDeployPkg)) { | ||
var debugMode = tl.getVariable('system.debug'); | ||
if(debugMode && debugMode.toLowerCase() == 'true') { | ||
tl.warning(tl.loc('AutoParameterizationMessage')); |
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.
Where is this message from? I can't see in task.json.
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.
Message is available in AzureRmWebAppDeployment/task.json
@@ -7,9 +7,18 @@ var jsonSubstitutionUtility = require('webdeployment-common/jsonvariablesubstitu | |||
var xmlSubstitutionUtility = require('webdeployment-common/xmlvariablesubstitutionutility.js'); | |||
var xdtTransformationUtility = require('webdeployment-common/xdttransformationutility.js'); | |||
|
|||
export function fileTransformations(isFolderBasedDeployment: boolean, JSONFiles: any, xmlTransformation: boolean, xmlVariableSubstitution: boolean, folderPath: string) { | |||
export function fileTransformations(isFolderBasedDeployment: boolean, JSONFiles: any, xmlTransformation: boolean, xmlVariableSubstitution: boolean, folderPath: string, webDeployPkg: 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.
why do you need webDeployPkg itself instead of passing a Boolean of isMSDeployPackage?
… users/vinca/changeDeploymentScriptLogic
if(extensionId.startsWith('python2')) { | ||
return homeDir + "Python27"; | ||
} | ||
else if(extensionId.startsWith('python351') || extensionId.startsWith('python352')) { |
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 may not be required as we are not showing option to install 3.5 < python version < 3.5.3 ?
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.
Additional check, if we add them.
No description provided.