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.

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

[EngSys] Allow Project Exclusions #17658

Merged
merged 1 commit into from
Jan 4, 2021
Merged

[EngSys] Allow Project Exclusions #17658

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
10 changes: 7 additions & 3 deletions eng/pipelines/templates/jobs/archetype-sdk-client.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ jobs:
dotnet pack eng/service.proj -warnaserror
/p:ServiceDirectory=${{ parameters.ServiceDirectory }}
/p:IncludeTests=false
/p:ExcludeProjects="${{ join('|', parameters.ExcludeProjects) }}"
Copy link
Member Author

@jsquire jsquire Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Though the expression is used in a couple of call sites, I chose not to move it into a variable; my goal was to be explicit and clear on how the value is formed, since there is string parsing taking place in the project file. I feel that having the join inline improves locality and prevents having to scan the file to understand what is happening.

/p:PublicSign=false $(VersioningProperties)
/p:Configuration=$(BuildConfiguration)
/p:CommitSHA=$(Build.SourceVersion)
Expand All @@ -58,7 +59,7 @@ jobs:
inputs:
artifactName: packages
path: $(Build.ArtifactStagingDirectory)

- template: /eng/pipelines/templates/steps/create-apireview.yml
parameters:
Artifacts: ${{parameters.Artifacts}}
Expand Down Expand Up @@ -102,7 +103,9 @@ jobs:
DOTNET_CLI_TELEMETRY_OPTOUT: 1
inputs:
filePath: "eng/scripts/CodeChecks.ps1"
arguments: -ServiceDirectory ${{parameters.ServiceToTest}}
arguments: >
-ServiceDirectory ${{parameters.ServiceToTest}}
-ExcludeProjects "${{ join('|', parameters.ExcludeProjects) }}"
pwsh: true
failOnStderr: false
- task: ms.vss-governance-buildtask.governance-build-task-component-detection.ComponentGovernanceComponentDetection@0
Expand Down Expand Up @@ -160,10 +163,11 @@ jobs:
${{ pair.key }}: ${{ pair.value }}
- template: /eng/pipelines/templates/steps/install-dotnet.yml
- script: >-
dotnet test eng/service.proj --filter TestCategory!=Live --framework $(TestTargetFramework)
dotnet test eng/service.proj --filter "(TestCategory!=Manually) & (TestCategory!=Live)" --framework $(TestTargetFramework)
Copy link
Member Author

Choose a reason for hiding this comment

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

The Manually filter existed in the tests archtype; it seemed like it should be applied here as well.

--logger "trx;LogFileName=$(TestTargetFramework).trx" --logger:"console;verbosity=normal"
/p:ServiceDirectory=${{parameters.ServiceToTest}}
/p:IncludeSrc=false /p:IncludeSamples=false /p:IncludePerf=false /p:IncludeStress=false
/p:ExcludeProjects="${{ join('|', parameters.ExcludeProjects) }}"
/p:Configuration=$(BuildConfiguration) $(ConvertToProjectReferenceOption)
/p:CollectCoverage=$(CollectCoverage) /p:CodeCoverageDirectory=${{parameters.ServiceDirectory}}
displayName: "Build & Test ($(TestTargetFramework))"
Expand Down
4 changes: 4 additions & 0 deletions eng/pipelines/templates/jobs/archetype-sdk-tests-jobs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ parameters:
- name: ServiceDirectory
type: string
default: not-specified
- name: ExcludeProjects
type: object
default: []
- name: TestSetupSteps
type: stepList
default: []
Expand Down Expand Up @@ -95,6 +98,7 @@ jobs:
/p:ServiceDirectory=${{ parameters.ServiceDirectory }}
/p:IncludeSrc=false /p:IncludeSamples=false /p:IncludePerf=false /p:IncludeStress=false
/p:BuildInParallel=${{ parameters.BuildInParallel }}
/p:ExcludeProjects="${{ join('|', parameters.ExcludeProjects) }}"
$(AdditionalTestArguments)

displayName: "Build & Test (all tests for $(TestTargetFramework))"
Expand Down
6 changes: 5 additions & 1 deletion eng/pipelines/templates/stages/archetype-sdk-client.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ parameters:
- name: ServiceDirectory
type: string
default: not-specified
- name: ExcludeProjects
type: object
default: []
- name: ServiceToTest
type: string
default: ''
Expand All @@ -38,6 +41,7 @@ stages:
parameters:
ServiceToTest: ${{ coalesce(parameters.ServiceToTest, parameters.ServiceDirectory) }}
ServiceDirectory: ${{ parameters.ServiceDirectory }}
ExcludeProjects: ${{ parameters.ExcludeProjects }}
Artifacts: ${{ parameters.Artifacts }}
TestPipeline: ${{ parameters.TestPipeline }}
ArtifactName: packages
Expand All @@ -53,4 +57,4 @@ stages:
TestPipeline: ${{ parameters.TestPipeline }}
ArtifactName: packages
TargetDocRepoOwner: ${{ parameters.TargetDocRepoOwner }}
TargetDocRepoName: ${{ parameters.TargetDocRepoName }}
TargetDocRepoName: ${{ parameters.TargetDocRepoName }}
4 changes: 4 additions & 0 deletions eng/pipelines/templates/stages/archetype-sdk-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ parameters:
- name: ServiceDirectory
type: string
default: not-specified
- name: ExcludeProjects
type: object
default: []
- name: TestSetupSteps
type: stepList
default: []
Expand Down Expand Up @@ -128,6 +131,7 @@ stages:
TimeoutInMinutes: ${{ parameters.TimeoutInMinutes }}
Location: ${{ parameters.Location }}
ServiceDirectory: ${{ parameters.ServiceDirectory }}
ExcludeProjects: ${{ parameters.ExcludeProjects }}
TestSetupSteps: ${{ parameters.TestSetupSteps }}
Platforms:
# Enumerate platforms and additional platforms based on supported clouds (sparse platform<-->cloud matrix).
Expand Down
14 changes: 9 additions & 5 deletions eng/scripts/CodeChecks.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@
param (
[Parameter(Position=0)]
[string] $ServiceDirectory,
[string] $ProjectDirectory

[Parameter()]
[string] $ProjectDirectory,

[Parameter()]
[string] $ExcludeProjects
)

$ErrorActionPreference = 'Stop'
Set-StrictMode -Version 1


[string[]] $errors = @()

function LogError([string]$message) {
Expand Down Expand Up @@ -72,21 +76,21 @@ try {

Write-Host "Re-generating clients"
Invoke-Block {
& dotnet msbuild $PSScriptRoot\..\service.proj /restore /t:GenerateCode /p:ServiceDirectory=$ServiceDirectory
& dotnet msbuild $PSScriptRoot\..\service.proj /restore /t:GenerateCode /p:ServiceDirectory=$ServiceDirectory /p:ExcludeProjects="$ExcludeProjects"

# https://github.com/Azure/azure-sdk-for-net/issues/8584
# & $repoRoot\storage\generate.ps1
}
}

Write-Host "Re-generating readmes"
Write-Host "Re-generating snippets"
Invoke-Block {
& $PSScriptRoot\Update-Snippets.ps1 -ServiceDirectory $ServiceDirectory
}

Write-Host "Re-generating listings"
Invoke-Block {
& $PSScriptRoot\Export-API.ps1 -ServiceDirectory $ServiceDirectory
& $PSScriptRoot\Export-API.ps1 -ServiceDirectory $ServiceDirectory -ExcludeProjects "$ExcludeProjects"
}

if (-not $ProjectDirectory)
Expand Down
5 changes: 3 additions & 2 deletions eng/scripts/Export-API.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
param (
[Parameter(Position=0)]
[ValidateNotNullOrEmpty()]
[string] $ServiceDirectory
[string] $ServiceDirectory,
[string] $ExcludeProjects
)

$servicesProj = Resolve-Path "$PSScriptRoot/../service.proj"

dotnet build /p:GenerateApiListingOnBuild=true /p:RunApiCompat=false /p:GeneratePackageOnBuild=false /p:Configuration=Release /p:IncludeSamples=false /p:IncludePerf=false /p:IncludeStress=false /p:IncludeTests=false /p:Scope="$ServiceDirectory" /restore $servicesProj
dotnet build /p:GenerateApiListingOnBuild=true /p:RunApiCompat=false /p:GeneratePackageOnBuild=false /p:Configuration=Release /p:IncludeSamples=false /p:IncludePerf=false /p:IncludeStress=false /p:IncludeTests=false /p:Scope="$ServiceDirectory" /p:ExcludeProjects="$ExcludeProjects" /restore $servicesProj
24 changes: 17 additions & 7 deletions eng/service.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,35 @@
<IncludeStress Condition="'$(IncludeStress)' == ''">true</IncludeStress>
<IncludeSamplesApplications Condition="'$(IncludeSamplesApplications)' == ''">true</IncludeSamplesApplications>
<IncludeSamplesApplications Condition="'$(ServiceDirectory)' != '*' or '$(IncludeSamples)' == 'false'">false</IncludeSamplesApplications>
<ExcludeProjectsDelimiter Condition="'$(ExcludeProjectsDelimiter)' == ''">|</ExcludeProjectsDelimiter>
<TraversalGlobalProperties>
CodeCoverageDirectory=$([System.IO.Path]::GetFullPath("$(CodeCoverageDirectory)", "$(MSBuildThisFileDirectory)..\sdk"));
</TraversalGlobalProperties>
</PropertyGroup>

<ItemGroup>
<ExcludeMgmtLib Include="..\sdk\*\Microsoft.*.Management.*\**\*.csproj;..\sdk\*mgmt*\**\*.csproj;" />
<MgmtExcludePaths Include="$(MSBuildThisFileDirectory)..\sdk\*\Microsoft.*.Management.*\**\*.csproj;$(MSBuildThisFileDirectory)..\sdk\*mgmt*\**\*.csproj" />
<TestProjects Include="..\sdk\$(ServiceDirectory)\**\tests\**\*.csproj" />
<SamplesProjects Include="..\sdk\$(ServiceDirectory)\**\samples\**\*.csproj" />
<PerfProjects Include="..\sdk\$(ServiceDirectory)\**\perf\**\*.csproj" />
<StressProjects Include="..\sdk\$(ServiceDirectory)\**\stress\**\*.csproj" />
<SampleApplications Include="..\samples\**\*.csproj" />
<SrcProjects Include="..\sdk\$(ServiceDirectory)\**\*.csproj" Exclude="@(TestProjects);@(SamplesProjects)"/>
<ProjectReference Include="@(TestProjects)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludeTests)' == 'true'" />
<ProjectReference Include="@(SamplesProjects)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludeSamples)' == 'true'" />
<ProjectReference Include="@(PerfProjects)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludePerf)' == 'true'" />
<ProjectReference Include="@(StressProjects)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludeStress)' == 'true'" />
<ProjectReference Include="@(SampleApplications)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludeSamplesApplications)' == 'true'"/>
<ProjectReference Include="@(SrcProjects)" Exclude="@(ExcludeMgmtLib)" Condition="'$(IncludeSrc)' == 'true'" />
<ProjectReference Include="@(TestProjects)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludeTests)' == 'true'" />
<ProjectReference Include="@(SamplesProjects)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludeSamples)' == 'true'" />
<ProjectReference Include="@(PerfProjects)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludePerf)' == 'true'" />
<ProjectReference Include="@(StressProjects)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludeStress)' == 'true'" />
<ProjectReference Include="@(SampleApplications)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludeSamplesApplications)' == 'true'"/>
<ProjectReference Include="@(SrcProjects)" Exclude="@(MgmtExcludePaths)" Condition="'$(IncludeSrc)' == 'true'" />
</ItemGroup>

<!--
Override project inclusions and force removal of those that have been explicitly excluded by the associated parameter.
Assume each item identifies a project name under the service directory and build the appropriate path mask.
-->
<ItemGroup Condition="'$(ExcludeProjects)' != ''">
<_ExcludePathsSource Include="$(ExcludeProjects.Split('$(ExcludeProjectsDelimiter)'))" />
<ProjectReference Remove="@(_ExcludePathsSource -> '$(MSBuildThisFileDirectory)..\sdk\$(ServiceDirectory)\%(Identity)\**\*.csproj')" />
</ItemGroup>

<Import Project="..\sdk\$(ServiceDirectory)\*.projects" />
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm a little late to the party here but for project exclusions it was my goal that individual service directories could include a .projects file (example https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/search/service.projects). That way the services could apply whatever logic they wanted in msbuild to exclude/include different projects based on whatever condition was interesting to them. Doing it this way would prevent to pass a set of project paths but instead could just be controlled by a simple boolean condition.

Copy link
Member Author

@jsquire jsquire Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'll take a look at this tomorrow and see if I can pivot to that, if that is the preference. Based on our initial short conversation, my goal was to mimic the exclude style from the CI pipelines so that it presented a consistent experience for the functionality.

Expand Down
55 changes: 55 additions & 0 deletions sdk/eventhub/ci.legacy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# NOTE: Please refer to https://aka.ms/azsdk/engsys/ci-yaml before editing this file.
Copy link
Member

Choose a reason for hiding this comment

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

We have tried to avoid using terms like legacy. If we are to follow our other naming conventions we should call these ci.data.yml to represent the track 1 data plane libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, "legacy" is the terminology that was agreed upon with the service team. I'd prefer to keep the nomenclature that is used within the ecosystem to avoid confusion. When I looked through the pipelines, most services with multiples appear to be split between "data" and "mgmt"; I never would have guessed that "data" was referring to T1.

Copy link
Member

Choose a reason for hiding this comment

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

yeah we were trying to keep "empty" (i.e. ci.yml) as the forward looking track 2 set, also sometimes called client and then for any track1 data plane we were using "ci.data.yml" or "ci.mgmt.yml" for mgmt.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I'm being honest, I find that pattern confusing, but I don't feel strongly enough to swim upstream. I'll adjust to the recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

There is no great convention but I believe it is better then using the term "legacy" and it at least has some consistency across different areas and languages.


trigger:
branches:
include:
- master
- hotfix/*
- release/*
paths:
include:
- sdk/eventhub/
exclude:
- sdk/eventhub/Azure.Messaging.EventHubs
- sdk/eventhub/Azure.Messaging.EventHubs.Processor
- sdk/eventhub/Azure.Messaging.EventHubs.Shared
- sdk/eventhub/Azure.ResourceManager.EventHubs
- sdk/eventhub/Microsoft.Azure.Management.EventHub
- sdk/eventhub/Microsoft.Azure.WebJobs.Extensions.EventHubs

pr:
branches:
include:
- master
- feature/*
- hotfix/*
- release/*
paths:
include:
- sdk/eventhub/
exclude:
- sdk/eventhub/Azure.Messaging.EventHubs
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned with having to keep track of the projects/paths in multiple places and keep it all in sync without making mistakes. Is there maybe some import voodoo we can use to centralize this information or write a script that would make sure things are up-to-date and correct?

Copy link
Member Author

@jsquire jsquire Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

These particular triggers are owned by DevOps, unfortunately, and aren't something that we have much influence over in terms of placement and syntax. We could use a sub template to import them into a variable, but this set is used in a single place, so there's little benefit.

The ExcludeProject parameter that we added has a different format with the goal of less ceremony and more flexibility. It's meant to be more of a "tell me the pattern of project names that you don't care about and we'll ignore anything under that." With the wildcard support, it shouldn't be necessary to change that often nor adapt to most changes in the repo paths.

To your point, the best way to centralize would likely be a script that could accept the wildcard patterns and generate the corresponding exclude paths. I think that's probably a bit of a premature optimization at this point, as there aren't many places where the T1 and T2 code is bound together under different ownership. For those that are, the T1 assets are in maintenance mode and it isn't likely that we'll see path changes. If we start to see people splitting pipelines in areas that are more volatile, I think that is a great time to revisit.

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'm going to commit this in the current form for now, to unblock me from splitting the Messaging libraries out. I'll file an issue to track considering a script to automate at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

Yes explicitly listing and maintaining these include/exclude paths has annoyed me as well. Unfortunately it is a devops limitation currently and so we just have to pay the cost until we can find a better solution.

I will say I do generally try and avoid mixing include and exclude path filters. I generally prefer we explicitly have an include list for the paths we want to trigger this. That way it is clearer what we expect to trigger this pipeline and only the people that care about this pipeline will need to edit it, as opposed to other folks that don't care about it but have to keep adding excludes to it. Of course another issue that will likely occur is that when a new directory is added someone will have to remember to add the path otherwise no pipeline will trigger.

Copy link
Member Author

@jsquire jsquire Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For this, I was following the example that had been shared with me during planning. Considering Wes' feedback, I'd prefer to have the inclusion by default with the rationale that its better to inadvertently include something that you need to exclude later than to forget to add something and not realize that it isn't included in CI.

- sdk/eventhub/Azure.Messaging.EventHubs.Processor
- sdk/eventhub/Azure.Messaging.EventHubs.Shared
- sdk/eventhub/Azure.ResourceManager.EventHubs
- sdk/eventhub/Microsoft.Azure.Management.EventHub
- sdk/eventhub/Microsoft.Azure.WebJobs.Extensions.EventHubs

extends:
template: ../../eng/pipelines/templates/stages/archetype-sdk-client.yml
parameters:
ServiceDirectory: eventhub
ExcludeProjects:
- Azure.ResourceManager.EventHubs
- Azure.Messaging.EventHubs
- Azure.Messaging.EventHubs.Processor
- Azure.Messaging.EventHubs.Shared
- Azure.Messaging.EventHubs.*
Copy link
Member Author

Choose a reason for hiding this comment

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

The wildcard should be sufficient, but there's one outstanding observation from the initial round of testing where it's not 100% confirmed for one of our script calls.

My approach here was to explicitly define everything that exists today and leave the wild card to provide some level of filtering for anything new. I'm still working with members of the engineering team to investigate and will open a new PR to trim to just the wild card, if possible.

ArtifactName: packages
Artifacts:
- name: Microsoft.Azure.EventHubs
safeName: MicrosoftAzureEventHubs
- name: Microsoft.Azure.EventHubs.Processor
safeName: MicrosoftAzureEventHubsProcessor
- name: Microsoft.Azure.EventHubs.ServiceFabricProcessor
safeName: MicrosoftAzureEventHubsServiceFabricProcessor
15 changes: 15 additions & 0 deletions sdk/eventhub/tests.legacy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
trigger: none

extends:
template: ../../eng/pipelines/templates/stages/archetype-sdk-tests.yml
parameters:
MaxParallel: 6
ServiceDirectory: eventhub
TimeoutInMinutes: 190
ExcludeProjects:
- Azure.ResourceManager.EventHubs
- Azure.Messaging.EventHubs
- Azure.Messaging.EventHubs.Processor
- Azure.Messaging.EventHubs.Shared
- Azure.Messaging.EventHubs.*
Clouds: 'Public,Canary'