Skip to content

Publish artifacts and symbols one file at a time - #7086

Merged
epananth merged 39 commits into
dotnet:mainfrom
epananth:publishing-pr
Apr 12, 2021
Merged

Publish artifacts and symbols one file at a time#7086
epananth merged 39 commits into
dotnet:mainfrom
epananth:publishing-pr

Conversation

@epananth

@epananth epananth commented Mar 10, 2021

Copy link
Copy Markdown
Member

To double check:

Issue -> #6987

Both the old functionality to download the entire artifact folder is still present and can be turned on by enabling the artifact download task and switching UseStreamingPublishing= false in the publish-assets.yml

There is definitely some redundant code, I'm hoping code review will help me make it better.

NOTE : PdbArtifacts are still downloaded like we did earlier, cos there is not way to get the file names before hand to download them from PdbArtifacts directory.

@epananth epananth changed the title Publishing pr Publish artifacts and symbols one file at a time Mar 10, 2021
@epananth
epananth marked this pull request as ready for review March 16, 2021 07:51
@epananth

Copy link
Copy Markdown
Member Author

@epananth

Copy link
Copy Markdown
Member Author

I ran tests for different promotions parallelly, and they all succeeded. I have also added retry logic for downloading files. So I think I covered most of the tests, we had discussed.

Comment thread eng/publishing/v3/publish-assets.yml Outdated

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

First round of feedback. Still need to finish looking at all the files.

Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/BuildArtifacts.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/BuildArtifacts.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Second pass. Two files to go

Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated
@epananth
epananth requested review from a user and michellemcdaniel and removed request for a user April 5, 2021 18:23
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs Outdated

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I only have a couple more nits and comments but provided you have some test builds with these latest changes that we can take a look at, this LGTM,

/// </summary>
/// <param name="client">Azdo client</param>
/// <param name="artifact">If it is PackageArtifacts or BlobArtifacts</param>
/// <param name="ArtifactName">If it is PackageArtifacts or BlobArtifacts</param>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: the parameter name is artifactName (casing)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated this


}

private async Task PublishPackagesToAzureStorageNugetByDownloadingEntireFolderAsync(HashSet<PackageArtifactModel> packagesToPublish,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

YAY!

@mmitche mmitche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for iterating on this. Complicated stuff.

@michellemcdaniel michellemcdaniel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm happy with how it looks in the staging pipeline (the only failures make sense/are because the files had already been uploaded since we tried on a bar id that already has been published). LGTM

@epananth

epananth commented Apr 6, 2021

Copy link
Copy Markdown
Member Author

@epananth

epananth commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

I did some test against installer and I am currently facing some IO exception, used by another process for blob publishing. I am investigating this issue, so I am not going to merge this PR till that issue is fixed.

@epananth

epananth commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

In the last change, I added the following

  1. Added await to CompareLocalPackageToFeedPackage
  2. Making sure PackageArchiveReader was using the correct scope
  3. added retry to ContainerIdAsync
  4. More logs for downloading file
  5. Added retry to directory delete
  6. Updated the test

@epananth
epananth requested review from a user and michellemcdaniel April 8, 2021 23:39
@epananth

epananth commented Apr 9, 2021

Copy link
Copy Markdown
Member Author

@mmitche mentioned my change is good to merge. :)

@epananth

epananth commented Apr 9, 2021

Copy link
Copy Markdown
Member Author

@epananth
epananth merged commit 23b4c45 into dotnet:main Apr 12, 2021
akoeplinger pushed a commit to akoeplinger/arcade that referenced this pull request Apr 12, 2021
@epananth
epananth deleted the publishing-pr branch April 15, 2021 05:30
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.

3 participants