Skip to content

[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

Merged

Conversation

github-actions[bot]
Copy link
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
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
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
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.

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