Skip to content

[release/9.0.1xx] Check shipping packages for poison#47224

Merged
NikolaMilosavljevic merged 3 commits intorelease/9.0.1xxfrom
backport/pr-47176-to-release/9.0.1xx
Mar 3, 2025
Merged

[release/9.0.1xx] Check shipping packages for poison#47224
NikolaMilosavljevic merged 3 commits intorelease/9.0.1xxfrom
backport/pr-47176-to-release/9.0.1xx

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot commented Mar 3, 2025

Backport of #47176 to release/9.0.1xx

/cc @NikolaMilosavljevic

@github-actions github-actions bot requested review from a team as code owners March 3, 2025 16:38
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Mar 3, 2025
Comment on lines +117 to +118
<AssetToCheck Include="$(ArtifactsAssetsDir)*$(ArchiveExtension)" />
<AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" />
Copy link
Copy Markdown
Member

@ViktorHofer ViktorHofer Mar 3, 2025

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.

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.

Thanks. I'd like to minimize the changes in this backport, especially as this was already the model before my PR.

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.

Sure. Feel free to consider this for the PR targeting main.

<AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" />
<!-- Include shipping nuget packages. -->
<PoisonFileToCheck Include="$(ArtifactsShippingPackagesDir)*.nupkg" />
<ShippingPackageToCheck Include="$(ArtifactsShippingPackagesDir)**/*.nupkg" />
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.

super tiny nit: inconsistent use path separator char. L122 uses \ which I would go with.

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.

This whole targets file is Linux specific, so / isn't out of place, i.e.

<IntermediateSymbol Include="$(IntermediateSymbolsRootDir)**/*" />

@NikolaMilosavljevic NikolaMilosavljevic merged commit a1cfcc5 into release/9.0.1xx Mar 3, 2025
33 checks passed
@NikolaMilosavljevic NikolaMilosavljevic deleted the backport/pr-47176-to-release/9.0.1xx branch March 3, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants