Surfaced SetParamFile in AzureRMWebDeploy task #1748

Merged
merged 2 commits into from May 26, 2016

Conversation

Projects
None yet
4 participants
@colindembovsky
Contributor

colindembovsky commented May 18, 2016

I often use a SetParameters.xml file with WebDeploy. Instead of adding it as an additional arg, I've surfaced this as an optional arg on the task. This allows the user to browse the path easily in the UI.
setparams

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas May 18, 2016

Hi @colindembovsky, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

Hi @colindembovsky, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@colindembovsky colindembovsky changed the title from Surfaced SetParamFile in task to Surfaced SetParamFile in AzureRMWebDeploy task May 18, 2016

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas May 18, 2016

@colindembovsky, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@colindembovsky, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@@ -82,7 +88,12 @@ $ErrorActionPreference = 'Stop'
$msDeployExePath = Get-MsDeployExePath
# Ensure that at most a package (.zip) file is found
-$packageFile = Get-SinglePackageFile -package $Package
+$packageFile = Get-SingleFilePath -file $Package

This comment has been minimized.

@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

This can be renamed to $packageFilePath to be consistent with the newly added variable

@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

This can be renamed to $packageFilePath to be consistent with the newly added variable

This comment has been minimized.

@colindembovsky

colindembovsky May 18, 2016

Contributor

I've renamed the var

@colindembovsky

colindembovsky May 18, 2016

Contributor

I've renamed the var

@@ -126,6 +127,10 @@ function Get-MsDeployCmdArgs
$msDeployCmdArgs += [String]::Format(' -skip:Directory="\\App_Data"')
}
+ if( -not [String]::IsNullOrEmpty($setParametersFile)){

This comment has been minimized.

@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

This may never be NullOrEmpty because of the default behavior of filePath type inputs.
FilePathType inputs default to SYSTEM_DEFAULTWORKINGDIRECTORY. So, we should check the value and reset it to "".

Ref: $publishProfile input in https://github.com/Microsoft/vsts-tasks/blob/master/Tasks/SqlServerDacpacDeployment/DeployToSqlServer.ps1

@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

This may never be NullOrEmpty because of the default behavior of filePath type inputs.
FilePathType inputs default to SYSTEM_DEFAULTWORKINGDIRECTORY. So, we should check the value and reset it to "".

Ref: $publishProfile input in https://github.com/Microsoft/vsts-tasks/blob/master/Tasks/SqlServerDacpacDeployment/DeployToSqlServer.ps1

This comment has been minimized.

@colindembovsky

colindembovsky May 18, 2016

Contributor

I actually do this check in the main file (properly now) - by the time this script is called, the path will be empty if the user didn't specify a SetParamsFile.

@colindembovsky

colindembovsky May 18, 2016

Contributor

I actually do this check in the main file (properly now) - by the time this script is called, the path will be empty if the user didn't specify a SetParamsFile.

@KrishnaAdityaB

This comment has been minimized.

Show comment
Hide comment
@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

@colindembovsky Let me get back after getting this spec reviewed

Contributor

KrishnaAdityaB commented May 18, 2016

@colindembovsky Let me get back after getting this spec reviewed

@KrishnaAdityaB

This comment has been minimized.

Show comment
Hide comment
@KrishnaAdityaB

KrishnaAdityaB May 18, 2016

Contributor

Though we can check if parameters file is mentioned in both the new field and additionalArgs, IMO - we need not. Current check of null or empty on new field is good enough.


Refers to: Tasks/AzureRmWebAppDeployment/Utility.ps1:138 in 1cb0960. [](commit_id = 1cb0960, deletion_comment = False)

Contributor

KrishnaAdityaB commented May 18, 2016

Though we can check if parameters file is mentioned in both the new field and additionalArgs, IMO - we need not. Current check of null or empty on new field is good enough.


Refers to: Tasks/AzureRmWebAppDeployment/Utility.ps1:138 in 1cb0960. [](commit_id = 1cb0960, deletion_comment = False)

@KrishnaAdityaB KrishnaAdityaB self-assigned this May 18, 2016

+$packageFilePath = Get-SingleFilePath -file $Package
+
+# Since the SetParametersFile is optional, but it's a FilePath type, it will have the value System.DefaultWorkingDirectory when not specified
+if( $SetParametersFile -eq $env:SYSTEM_DEFAULTWORKINGDIRECTORY -or $SetParametersFile -eq [String]::Concat($env:SYSTEM_DEFAULTWORKINGDIRECTORY, "\")){

This comment has been minimized.

@colindembovsky

colindembovsky May 18, 2016

Contributor

I've added this check in to improve the "guard" of the empty SetParametersFile.

@colindembovsky

colindembovsky May 18, 2016

Contributor

I've added this check in to improve the "guard" of the empty SetParametersFile.

@codito codito assigned Ajay-MS and unassigned KrishnaAdityaB May 26, 2016

@Ajay-MS Ajay-MS merged commit d7ca76d into Microsoft:master May 26, 2016

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