Skip to content

DependentPackages -> CanaryPackages #8866

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.

Sign up for GitHub

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

Closed
scbedd wants to merge 1 commit into from
Closed

DependentPackages -> CanaryPackages #8866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions eng/common/scripts/Package-Properties.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class PackageProps
[boolean]$IsNewSdk
[string]$ArtifactName
[string]$ReleaseStatus
[string[]]$DependentPackages
[string[]]$CanaryPackages

PackageProps([string]$name, [string]$version, [string]$directoryPath, [string]$serviceDirectory)
{
Expand Down Expand Up @@ -129,19 +129,30 @@ function Get-PrPkgProperties([string]$InputDiffJson) {
if ($shouldInclude) {
$packagesWithChanges += $pkg

if ($pkg.DependentPackages) {
$dependentPackagesForInclusion += $pkg.DependentPackages
if ($pkg.CanaryPackages) {
$dependentPackagesForInclusion += $pkg.CanaryPackages
}
}
}
}

foreach ($addition in $dependentPackagesForInclusion) {
if ($lookup[$addition]) {
$packagesWithChanges += $lookup[$addition]
# there is a custom function to get dependent packages from a changed package set and diff
if ($DependentPackagesFromPackageSetFn -and (Test-Path "Function:$DependentPackagesFromPackageSetFn"))
{
$packagesWithChanges += &$DependentPackagesFromPackageSetFn $packagesWithChanges $diff
Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 21, 2024

Choose a reason for hiding this comment

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

This function should be implemented in Language-Settings.ps1. It accepts the pkgProps list for the detected packages that were included in the change, and the pscustomobject representation of the diff.json.

Copy link
Copy Markdown
Member

@hallipr hallipr Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you need to work with $dependentPackagesForInclusion and $pkg.CanaryPackages above if the language overrides "Get-${Language}-DependentPackagesFromPackageSet"

Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 21, 2024

Choose a reason for hiding this comment

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

I don't understand what you're asking. We're giving the function A) the packages props that were changed (first arg) and B) we're handing you the package diff itself. I'm doing that for additional customization if you want it.

Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. You're saying

why not offer an override for this entire function

Because Wes didn't want me to, I started with a per language function for Get-PrPkgProperties stored in Language-Settings at the very beginning of this process.

Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 23, 2024

Choose a reason for hiding this comment

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

Aha. You're asking if we want to interact with the canary packages at all if the function is defined. The answer is NO. I'll remove the add from the function above if the function is defined.

}
else {
foreach ($addition in $dependentPackagesForInclusion) {
Copy link
Copy Markdown
Member

@hallipr hallipr Aug 21, 2024

Choose a reason for hiding this comment

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

You could remove the variable $dependentPackagesForInclusion and pull directly from packages with changes here:

foreach ($addition in $packagesWithChanges.CanaryPackages) {

Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We'd still need two.

foreach ($packageWithChanges in $pkgProps)
  foreach($addition in $packageWithChanges.CanaryPackages)

Cause we try to check to see if it's real. I see where you're coming from where we could just add them as a path without looking up, but why? You're going to be able to override the function anyway so this won't matter to you as much.

$key = $addition.Replace($RepoRoot, "").SubString(1)

if ($lookup[$key]) {
$packagesWithChanges += $lookup[$key]
}
}
}



return $packagesWithChanges
}

Expand Down
1 change: 1 addition & 0 deletions eng/common/scripts/common.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ $GetEmitterNameFn = "Get-${Language}-EmitterName"
$GetDirectoriesForGenerationFn = "Get-${Language}-DirectoriesForGeneration"
$UpdateGeneratedSdksFn = "Update-${Language}-GeneratedSdks"
$IsApiviewStatusCheckRequiredFn = "Get-${Language}-ApiviewStatusCheckRequirement"
$DependentPackagesFromPackageSetFn = "Get-${Language}-DependentPackagesFromPackageSet"
Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 22, 2024

Choose a reason for hiding this comment

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

Ok I've re-added trailing whitespace in a rebase so this should actually push out now.

Copy link
Copy Markdown
Member Author

@scbedd scbedd Aug 23, 2024

Choose a reason for hiding this comment

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

Frustration. Still breaks on patch apply.


# Expected to be set in eng/scripts/docs/Docs-Onboarding.ps1
$SetDocsPackageOnboarding = "Set-${Language}-DocsPackageOnboarding"
Expand Down
⚡