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

Support artifacts published from any location #6268

Merged
merged 3 commits into from
Mar 15, 2018

Conversation

NCarlsonMSFT
Copy link
Member

Fixes #5736

$oldAppPackagePath = Join-Path $oldDropLocation $newAppPackagePath.SubString((Get-VstsTaskVariable -Name Build.SourcesDirectory -Require).Length + 1)
if (-not $newAppPackagePath.StartsWith($pkgArtifactPath))
{
Write-Error (Get-VstsLocString -Key PackageIsNotUnderArtifact -ArgumentList @($newAppPackagePath, $pkgArtifactPath))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Write-Error effectively throws an exception an halts execution

@@ -57,20 +62,26 @@

Write-Host (Get-VstsLocString -Key SearchingApplicationType -ArgumentList $appTypeName)

$oldAppPackagePath = Join-Path $oldDropLocation $newAppPackagePath.SubString((Get-VstsTaskVariable -Name Build.SourcesDirectory -Require).Length + 1)
if (-not $newAppPackagePath.StartsWith($pkgArtifactPath))
Copy link
Contributor

Choose a reason for hiding this comment

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

It is very difficult to make user understand about this new mandatory input. Can we think of any alternative without additional input. Can we parse $newAppPackagePath to check if it contains a path fragment which matches path inside $oldPackagePath. For example, if new path is C:\a\b\c\ApplicationPackage.xml then we can iterate whether old package contains any of following path:

  • a\b\c\ApplicationPackage.xml
  • b\c\ApplicationPackage.xml
  • c\ApplicationPackage.xml
  • ApplicationPackage.xml

@bishal-pdMSFT
Copy link
Contributor

@NCarlsonMSFT are you planning to take this PR forward?

@NCarlsonMSFT
Copy link
Member Author

@bishal-pdMSFT I'd like to, I was waiting on you to review the updates per your comments.

@NCarlsonMSFT
Copy link
Member Author

@bishal-pdMSFT are there any other questions/concerns you have before merging this?

$oldAppManifestPath = Join-Path $oldAppPackagePath $appManifestName
if (Test-Path $oldAppManifestPath)
# Try and find the old app package path by finding the largest substring of the path that exists in the artifact path
$relativePath = $newAppPackagePath
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we start with Build.SourcesDirectory directory rather than root?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally this would actually be under Build.StagingDirectory, but there is no guarantee of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it

$relativePath.Trim([System.IO.Path]::DirectorySeparatorChar)
$oldAppPackagePath = Join-Path $oldDropLocation $relativePath
while(!(Test-Path $oldAppPackagePath))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have UTs for this non-trivial logic

@NCarlsonMSFT NCarlsonMSFT merged commit 4b243f6 into master Mar 15, 2018
@NCarlsonMSFT NCarlsonMSFT deleted the users/ncarlson/SupportNotStagingDirectory branch March 15, 2018 05:31
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.

5 participants