Skip to content

DependentPackages -> CanaryPackages#8866

Closed
scbedd wants to merge 1 commit intomainfrom
hotfix/adjustments-for-rust-pr-packages
Closed

DependentPackages -> CanaryPackages#8866
scbedd wants to merge 1 commit intomainfrom
hotfix/adjustments-for-rust-pr-packages

Conversation

@scbedd
Copy link
Copy Markdown
Member

@scbedd scbedd commented Aug 21, 2024

Also allow a custom dependentPackages function so that @hallipr can easily override.

@scbedd scbedd self-assigned this Aug 21, 2024
@scbedd scbedd requested a review from a team as a code owner August 21, 2024 20:29
# 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

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

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

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

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

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.

$packagesWithChanges += &$DependentPackagesFromPackageSetFn $packagesWithChanges $diff
}
else {
foreach ($addition in $dependentPackagesForInclusion) {
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.

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

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.

re-add some trailing whitespace so the diff handles better
@scbedd scbedd force-pushed the hotfix/adjustments-for-rust-pr-packages branch from 1d05d3f to cabd8bc Compare August 21, 2024 23:59
$GetDirectoriesForGenerationFn = "Get-${Language}-DirectoriesForGeneration"
$UpdateGeneratedSdksFn = "Update-${Language}-GeneratedSdks"
$IsApiviewStatusCheckRequiredFn = "Get-${Language}-ApiviewStatusCheckRequirement"
$DependentPackagesFromPackageSetFn = "Get-${Language}-DependentPackagesFromPackageSet"
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.

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

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.

Frustration. Still breaks on patch apply.

@scbedd
Copy link
Copy Markdown
Member Author

scbedd commented Aug 23, 2024

Closing in favor of #8878 trying to get this automation to work properly.

@scbedd scbedd closed this Aug 23, 2024
@scbedd scbedd deleted the hotfix/adjustments-for-rust-pr-packages branch October 7, 2024 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants