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

App Service Deploy 4.* Task Version (Preview) #6422

Merged
merged 20 commits into from
Mar 5, 2018

Conversation

vincent1173
Copy link
Contributor

@vincent1173 vincent1173 commented Feb 13, 2018

  • Update Help markdown

"Patch": 42
"Major": 4,
"Minor": 0,
"Patch": 0
},
"releaseNotes": "What's new in Version 3.0: <br/>&nbsp;&nbsp;Supports File Transformations (XDT) <br/>&nbsp;&nbsp;Supports Variable Substitutions(XML, JSON) <br/>Click [here](https://aka.ms/azurermwebdeployreadme) for more Information.",
Copy link
Member

Choose a reason for hiding this comment

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

Release notes has to be updated with latest breaking change details. Work with PM on this.

@@ -25,13 +26,13 @@
"name": "FileTransformsAndVariableSubstitution",
"displayName": "File Transforms & Variable Substitution Options",
"isExpanded": false,
"visibleRule": "WebAppKind != linux && WebAppKind != applinux && WebAppKind != \"\""
"visibleRule": "WebAppKind != app44linux && WebAppKind != app44linux44container && WebAppKind != \"\""
Copy link
Member

Choose a reason for hiding this comment

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

the key app44linux has to be changed based on the mail conversation we have had with @azooinmyluggage

"app": "Web App",
"applinux": "Linux Web App",
"app": "Web App on Windows",
"app44linux": "Web App on Linux with built-In Images",
Copy link
Member

Choose a reason for hiding this comment

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

Is the word 'built-In' used in Azure portal as well? also check for casing - b is small but I is caps.

"applinux": "Linux Web App",
"app": "Web App on Windows",
"app44linux": "Web App on Linux with built-In Images",
"app44linux44container": "Web App for Containers with Container Registry",
Copy link
Member

Choose a reason for hiding this comment

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

Check this with PMs. There are multiple Containers in the label...

@@ -22,7 +22,7 @@ export class ContainerBasedDeploymentUtility {
}

public async deployWebAppImage(taskParameters: TaskParameters): Promise<void> {
var imageName = this._getImageName();
let imageName: string = this._getDockerHubImageName();
Copy link
Member

Choose a reason for hiding this comment

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

@jikuma - Can you review the changes related container deployment.

StartupCommand: tl.getInput('StartupCommand', false),
WebAppUri: tl.getInput('WebAppUri', false),
ConfigurationSettings: tl.getInput('ConfigurationSettings', false)
}

taskParameters.WebAppKind = taskParameters.WebAppKind.replace(/44/g, ',');
Copy link
Member

Choose a reason for hiding this comment

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

This might changed based on feedback from @azooinmyluggage

@@ -22,7 +22,7 @@ export class ContainerBasedDeploymentUtility {
}

public async deployWebAppImage(taskParameters: TaskParameters): Promise<void> {
var imageName = this._getImageName();
let imageName: string = this._getDockerHubImageName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are jumping task major version here. There is an opportunity for us to bring docker tasks and azure web app task in sync with how they take image name as input. in Docker tasks there is only one input image name which includes repository name and tag while azure weapp task takes them separately.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the reason these are separate is because at some point we want to enable drop down for each of these inputs, which will not be possible if we take them as a single input.

@vincent1173 vincent1173 merged commit 87e2a54 into master Mar 5, 2018
@vincent1173
Copy link
Contributor Author

vincent1173 commented Mar 5, 2018

#5984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: AzureAppService Label to monitor Azure App Service issues Area: Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants