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

[ServiceFabricDeploy] Test application package only if copy and register is going to happen #7524

Merged
merged 5 commits into from
Jun 21, 2018

Conversation

bishal-pdMSFT
Copy link
Contributor

No description provided.

@@ -383,7 +384,7 @@ function Publish-UpgradedServiceFabricApplication
if ($UnregisterUnusedVersions)
{
Write-Host (Get-VstsLocString -Key SFSDK_UnregisterUnusedVersions)
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName | Where-Object { $_.ApplicationTypeVersion -ne $names.ApplicationTypeVersion })
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName)

Choose a reason for hiding this comment

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

rename variable $registeredAppTypes to $registeredAppType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#Fixed

@@ -383,7 +384,7 @@ function Publish-UpgradedServiceFabricApplication
if ($UnregisterUnusedVersions)
{
Write-Host (Get-VstsLocString -Key SFSDK_UnregisterUnusedVersions)
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName | Where-Object { $_.ApplicationTypeVersion -ne $names.ApplicationTypeVersion })
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName)
{
try
{

Choose a reason for hiding this comment

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

If business use case is to consume the error in case of AppType and Version in use then its better to check for error code "ApplicationTypeInUse" instead of consuming System.Fabric.FabricException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO use case is to suppress any kind of exception. We don't want task to fail due to failure here. Infact I will remove [System.Fabric.FabricException] from here as well

Choose a reason for hiding this comment

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

Lets communicate to the users by surfacing a warning message in case of failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -383,7 +384,7 @@ function Publish-UpgradedServiceFabricApplication
if ($UnregisterUnusedVersions)
{
Write-Host (Get-VstsLocString -Key SFSDK_UnregisterUnusedVersions)
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName | Where-Object { $_.ApplicationTypeVersion -ne $names.ApplicationTypeVersion })
foreach ($registeredAppTypes in Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $names.ApplicationTypeName)

Choose a reason for hiding this comment

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

Just for the info, please let me know the reason to remove 'Where-Object' clause from the query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$UnregisterUnusedVersions means that any unused type should be unregistered. We were excluding current version as it is being used in upgrade. However, the upgrade might fail and that means type is not in use. Hence, now we will try to unregister all types. If upgrade was successful, unregister to current type will fail - but that error will be supressed.

@bishal-pdMSFT bishal-pdMSFT merged commit 557ad98 into master Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants