Skip to content
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

net - pullrequest #47957

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

net - pullrequest #47957

wants to merge 8 commits into from

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented Jan 23, 2025

This PR...

  • Adds a new net - pullrequest build.
    • This PR dynamically expands/contracts to only build and test the changed packages.
    • If Azure.Core or Azure.ResourceManager are present in the set of packages that have been changed, their dependencies are also discovered and distributed sparsely to test runs. This completely replaces the static AdditionalDependency leg that currently exists only on net - core and net - resourcemanager. It utilizes exactly the same dependency discovery that original leg utilized. (eng/service.proj ProjectDependsOn target)
    • The dependency discovery is fairly slow still, so it's limited to Azure.Core/Azure.ResourceManager. That can be adjusted.

Todo:

  • Add language-settings function to handle which packages should be build/tested if a PR is purely non-sdk changes.
  • Fix the docs generation to honor the changed packages
  • Re-add dev versioning save of save-package-props
  • Fix existing build legs which are broken by the change to propertyoverridefile usage in build-test.
    • resourcemanager
    • core

revert updates to the azurite yml. re-add Install-Azurite. Make the installation part of only the build test stage
@scbedd scbedd self-assigned this Jan 23, 2025
@github-actions github-actions bot added the Storage Storage Service (Queues, Blobs, Files) label Jan 23, 2025
- pwsh: |
$services = "$(ChangedServices)" -split ","

foreach ($service in $services) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use the project override list for this step?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can. There's another place in the build stage that does this as well.

@@ -59,7 +59,8 @@
"SupportsRecording": true,
"CollectCoverage": true
}
}
},
"AdditionalTestArguments": "/p:UseProjectReferenceToAzureClients=false"
Copy link
Member

Choose a reason for hiding this comment

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

Did we intentionally want to add this to the include?

Copy link
Member Author

@scbedd scbedd Jan 23, 2025

Choose a reason for hiding this comment

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

I did. It's so that this value is present on the Include matrix run added to the matrix at the end. I'm just ensuring that the variable is available so that when we attempt to override it in the matrix (remember we re-use the matrix values) it just works.

@@ -62,6 +62,49 @@
<ItemGroup Condition="'$(ProjectListOverrideFile)' != '' ">
<Track1ProjectsToRemove Include="$(MSBuildThisFileDirectory)..\sdk\*\Microsoft.*.Management.*\**\*.csproj;$(MSBuildThisFileDirectory)..\sdk\*mgmt*\**\*.csproj;$(MSBuildThisFileDirectory)..\sdk\*\Microsoft.Azure.*\**\*.csproj" />
<ProjectReference Remove="@(Track1ProjectsToRemove)" />
<MgmtExcludePaths Include="$(MSBuildThisFileDirectory)..\sdk\*\Microsoft.*.Management.*\**\*.csproj;$(MSBuildThisFileDirectory)..\sdk\*mgmt*\**\*.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean none of these projects will be included in the PR pipeline? Also are these actual mgmt projects or are you just piggy backing on this include?

Copy link
Member Author

@scbedd scbedd Jan 23, 2025

Choose a reason for hiding this comment

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

Does this mean none of these projects will be included in the PR pipeline? Also are these actual mgmt projects or are you just piggy backing on this include?

I'm keeping these mgmt exclude paths because these exclusions are what are honored by default for most usages of the eng/service proj (targeted at a specific service directory).

Copy link
Member

Choose a reason for hiding this comment

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

If we need these now I wonder if there is a better way to share them with the list above instead of duplicating.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will pick on re-using the item group.


$setting = $pkgNames -join ","
Write-Host "Setting ProjectNames to: `n$setting"
Write-Host "##vso[task.setvariable variable=ProjectNames;]$setting"
Copy link
Member

Choose a reason for hiding this comment

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

Is the project names actually interesting for anything? I would expect folks to use the project overrides or just the packageinfos in most places.

Copy link
Member Author

@scbedd scbedd Jan 23, 2025

Choose a reason for hiding this comment

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

This is a good point. If we don't use it anywhere I'll clean this up.

Project Names is set by the matrix strategy for build-test. We need to keep this around for when a matrix isn't setting ProjectNames variable. (this would be in build/analyze stages).

$DependencyCalculationPackages = @(
"Azure.Core",
"Azure.Identity"#,
# "Azure.ResourceManager" temporarily removed until we can figure out why it's not working correctly on the build machine. it works locally
Copy link
Member Author

Choose a reason for hiding this comment

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

Going to fix this in next commit as well.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Overall looks reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
Status: 🔬 Dev in PR
Development

Successfully merging this pull request may close these issues.

3 participants