-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[release/9.0.1xx] Check shipping packages for poison #47224
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
[release/9.0.1xx] Check shipping packages for poison #47224
Conversation
<AssetToCheck Include="$(ArtifactsAssetsDir)*$(ArchiveExtension)" /> | ||
<AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Instead of the additional Remove
operation, I would add this as an Exclude
to L117.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'd like to minimize the changes in this backport, especially as this was already the model before my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Feel free to consider this for the PR targeting main.
<!-- Include shipping nuget packages. --> | ||
<PoisonFileToCheck Include="$(ArtifactsShippingPackagesDir)*.nupkg" /> | ||
<ShippingPackageToCheck Include="$(ArtifactsShippingPackagesDir)**/*.nupkg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super tiny nit: inconsistent use path separator char. L122 uses \
which I would go with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole targets file is Linux specific, so /
isn't out of place, i.e.
<IntermediateSymbol Include="$(IntermediateSymbolsRootDir)**/*" /> |
Backport of #47176 to release/9.0.1xx
/cc @NikolaMilosavljevic