-
Notifications
You must be signed in to change notification settings - Fork 839
Add AmbientMetadata.Build component #6623
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
base: main
Are you sure you want to change the base?
Add AmbientMetadata.Build component #6623
Conversation
I love porting until I realize my commit history is lost. 🤣 |
src/Libraries/Microsoft.Extensions.AmbientMetadata.Build/BuildMetadata.cs
Outdated
Show resolved
Hide resolved
// Azure DevOps environment variables: https://learn.microsoft.com/azure/devops/pipelines/build/variables#build-variables-devops-services | ||
internal static string? AzureBuildId = Environment.GetEnvironmentVariable("Build_BuildId"); | ||
internal static string? AzureBuildNumber = Environment.GetEnvironmentVariable("Build_BuildNumber"); | ||
internal static string? AzureSourceBranchName = Environment.GetEnvironmentVariable("Build_SourceBranchName"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this alternatively be fetched by the InitializeSourceControlInformation
and related targets in MSBuild that are used for SourceLink, that way this would work for local builds as well as CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to fetch data from MSBuild properties, which by default are initialized from environment variables.
…hub.com/evgenyfedorov2/dotnet-extensions into users/evgenyfedorov2/add_build_metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the empty src/Libraries/Microsoft.Extensions.AmbientMetadata.Build/Microsoft.Extensions.AmbientMetadata.Build.json
file?
src/Generators/Microsoft.Gen.BuildMetadata/BuildMetadataGenerator.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.BuildMetadata/Microsoft.Gen.BuildMetadata.csproj
Outdated
Show resolved
Hide resolved
Removed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new AmbientMetadata.Build component that provides automatic build metadata collection from CI/CD pipelines. The component uses source generation to extract build information at compile time and register it in the dependency injection container as IOptions.
Key changes:
- New BuildMetadata class with properties for build ID, number, branch name, and source version
- Source generator that reads MSBuild properties and generates configuration extensions
- Support for both Azure DevOps and GitHub Actions CI environments
Reviewed Changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Libraries/Microsoft.Extensions.AmbientMetadata.Build/BuildMetadata.cs | Defines the core data model for build metadata |
src/Generators/Microsoft.Gen.BuildMetadata/BuildMetadataGenerator.cs | Main source generator that creates build metadata extensions |
src/Libraries/Microsoft.Extensions.AmbientMetadata.Build/BuildMetadataServiceCollectionExtensions.cs | Extension methods for registering BuildMetadata in DI container |
src/Libraries/Microsoft.Extensions.AmbientMetadata.Build/buildTransitive/Microsoft.Extensions.AmbientMetadata.Build.props | MSBuild properties file that maps CI environment variables to build properties |
test/Libraries/Microsoft.Extensions.AmbientMetadata.Build.Tests/ | Unit tests for the build metadata functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...s/Microsoft.Extensions.AmbientMetadata.Build.Tests/ConfigurationBindingQuirkBehaviorTests.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.Extensions.AmbientMetadata.Build.Tests/ConfigurationBindingQuirkBehaviorTests.cs
Outdated
Show resolved
Hide resolved
…s/ConfigurationBindingQuirkBehaviorTests.cs Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...sions.AmbientMetadata.Build/buildTransitive/Microsoft.Extensions.AmbientMetadata.Build.props
Outdated
Show resolved
Hide resolved
9e1009a
to
7ed6565
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/Generators/Microsoft.Gen.BuildMetadata/AnalyzerConfigOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
@333fred, @jjonescz, @JoeRobich PTAL |
src/Generators/Microsoft.Gen.BuildMetadata/Microsoft.Gen.BuildMetadata.csproj
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.BuildMetadata/BuildMetadataGenerator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...s/Microsoft.Extensions.AmbientMetadata.Build.Tests/ConfigurationBindingQuirkBehaviorTests.cs
Show resolved
Hide resolved
...s/Microsoft.Extensions.AmbientMetadata.Build.Tests/ConfigurationBindingQuirkBehaviorTests.cs
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.BuildMetadata/BuildMetadataGenerator.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.BuildMetadata/AnalyzerConfigOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
context.AddSource("BuildMetadataExtensions.g.cs", SourceText.From(result, Encoding.UTF8)); | ||
} | ||
|
||
private readonly record struct BuildMetadata(string? BuildId, string? BuildNumber, string? SourceBranchName, string? SourceVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "IsAzureDevOps" flag be also stored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of today it is used for deciding azure devops vs. github actions, nothing else. It is not used as a public property either. So I don't see why we should store it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this source generator is about embedding metadata like build ID into the compilation, right? But you only embed "build ID", and then when I look into the DLL, I will see an attribute which tells me the "build ID" but I won't know if it was actually built on GitHub Actions or AzDo. (I guess I should know where my product is built, but maybe it can be built on either of these...) So I was just wondering if that would also be a useful piece of information to embed. But I may be completely misunderstanding the big picture here :)
Also I guess IsAzDo + BuildId is still not a unique identifier (to be able to unambiguously find the original build), one would also need to know the azdo org etc. So that's probably not the goal and you can ignore me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I won't know if it was actually built on GitHub Actions or AzDo
this is intended, this component tries to abstract away that stuff. In theory, we can be adding support of some other CI system in future
Also I guess IsAzDo + BuildId is still not a unique identifier
This is not the goal either. If users find it useful, we will consider adding it, but right now we try to limit the public API surface and make the component simple - it just gives users build information.
|
||
namespace Microsoft.Gen.BuildMetadata; | ||
|
||
[SuppressMessage("Format", "S1199", Justification = "For better visualization of how the generated code will look like.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this suppression still needed? (I'm not sure what S1199 is about actually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a Sonar rule https://sonarqube.inria.fr/sonarqube/coding_rules?open=csharpsquid%3AS1199&rule_key=csharpsquid%3AS1199, kicks in because we have extra curly braces
src/Generators/Microsoft.Gen.BuildMetadata/BuildMetadataGenerator.cs
Outdated
Show resolved
Hide resolved
// Create the generator driver with the options provider | ||
var driver = Microsoft.CodeAnalysis.CSharp.CSharpGeneratorDriver.Create( | ||
generators: new[] { new BuildMetadataGenerator().AsSourceGenerator() }, | ||
parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the source generators are supposed to run across various user configurations, they should support different language versions. I'm not sure if that's something worth testing though.
optionsProvider: optionsProvider); | ||
|
||
var result = driver.RunGenerators(comp!); | ||
var result = driver.RunGeneratorsAndUpdateCompilation(comp!, out var outputCompilation, out var diagnostics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diagnostics
is just the diagnostics your generator would have reported itself; the global::System.string
issue would not have been caught by this. Please verify that compilation.GetDiagnostics
either returns nothing, or the set of diagnostics you're expecting.
# About This Project | ||
|
||
This ambient metadata provider provides Build information at runtime. | ||
See https://learn.microsoft.com/azure/devops/pipelines/build/variables for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also mention https://docs.github.com/en/actions/reference/variables-reference#default-environment-variables here?
Adding a brand new component - Build metadata, which automatically grabs build information from the CI/CD pipelines, deserializes it into a strong type
BuildMetadata
and registers it in Dependency Injection container asIOptions<BuildMetadata>
.The component uses source generation to collect build information and immediately express it via C# code.
Initially, only GitHub Actions and Azure DevOps are supported
Microsoft Reviewers: Open in CodeFlow