Implement configuration ingestion #5636

adamzip · 2025-12-10T15:27:04Z

Copilot

Pull request overview

This PR implements configuration ingestion functionality to allow the Product Construction Service to validate and persist configuration data (subscriptions, channels, default channels, and branch merge policies) from external sources. The implementation introduces a new interface for externally synced entities, validation logic for entity uniqueness and field constraints, and CRUD operations for managing configuration data.

Key changes:

Introduces IExternallySyncedEntity<TId> interface to standardize entity identification across YAML models and database models
Implements comprehensive validation framework for configuration entities with uniqueness checking
Adds CRUD operations in SqlBarClient and ConfigurationIngestor to create, update, and delete configuration entities with proper namespace isolation

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/IExternallySyncedEntity.cs	New interface defining contract for externally synced entities with unique identifiers
src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/SubscriptionYaml.cs	Implements IExternallySyncedEntity using subscription Id as unique identifier
src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/ChannelYaml.cs	Implements IExternallySyncedEntity using channel name as unique identifier
src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/DefaultChannelYaml.cs	Implements IExternallySyncedEntity using repository/branch/channel tuple; adds Id field for ingestion
src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/BranchMergePoliciesYaml.cs	Implements IExternallySyncedEntity using repository/branch tuple as unique identifier
src/Maestro/Maestro.Data/Models/Repository.cs	Adds UniqueId property to RepositoryBranch for consistency with IExternallySyncedEntity pattern
src/Maestro/Maestro.Data/Models/DefaultChannel.cs	Implements IExternallySyncedEntity on database model for alignment with YAML model
src/Maestro/Maestro.DataProviders/ISqlBarClient.cs	Adds interface methods for subscription CRUD operations used in configuration ingestion
src/Maestro/Maestro.DataProviders/SqlBarClient.cs	Implements CRUD methods with conflict detection, validation, and conversion between client/DAO models
src/Maestro/Maestro.DataProviders/Exceptions/EntityConflictException.cs	New exception type for handling duplicate entity conflicts during ingestion
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/IConfigurationIngestor.cs	Updated interface signature to return entity changes and require namespace parameter
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationIngestor.cs	Core ingestion orchestration including validation, entity comparison, and database persistence
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationData.cs	Changed to use YAML models instead of DAO models for external configuration representation
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationDataUpdate.cs	New record types capturing created, updated, and removed entities for each configuration type
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationDataHelper.cs	Helper methods for computing entity differences and converting between YAML and DAO models
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/EntityValidator.cs	Generic validator for checking entity uniqueness across IExternallySyncedEntity collections
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/SubscriptionValidator.cs	Updated to validate collections and work with SubscriptionYaml instead of DAO models
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/ChannelValidator.cs	Updated to validate collections and work with ChannelYaml instead of DAO models
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/DefaultChannelValidator.cs	Updated to validate collections and work with DefaultChannelYaml instead of DAO models
src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/BranchMergePolicyValidator.cs	Updated to validate collections and work with BranchMergePoliciesYaml instead of DAO models

Comments suppressed due to low confidence (6)

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:641

This assignment assigns Enabled to itself.

        existingSubscription.SourceRepository = existingSubscription.SourceRepository;

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:642

This assignment assigns SourceEnabled to itself.

        existingSubscription.TargetRepository = existingSubscription.TargetRepository;

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:643

This assignment assigns SourceDirectory to itself.

        existingSubscription.Enabled = existingSubscription.Enabled;

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:644

This assignment assigns TargetDirectory to itself.

        existingSubscription.SourceEnabled = existingSubscription.SourceEnabled;

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:646

This assignment assigns PullRequestFailureNotificationTags to itself.

        existingSubscription.TargetDirectory = existingSubscription.TargetDirectory;

src/Maestro/Maestro.DataProviders/SqlBarClient.cs:546

The contents of this container are never initialized.

        var newSubscriptionHashes = new HashSet<string>();

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

src/Maestro/Maestro.DataProviders/ISqlBarClient.cs

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Validations/EntityValidator.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationIngestor.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

…urationDataHelper.cs Co-authored-by: Copilot <[email protected]>

Co-authored-by: Copilot <[email protected]>

…dEntity.cs Co-authored-by: Copilot <[email protected]>

Co-authored-by: Copilot <[email protected]>

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

test/Maestro.DataProviders.Tests/ConfigurationIngestionTests.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

- Make some internal things public - Fix id update bug with default channels - Add AssetFilter persistence for subscriptions - Change EF queries to make sure all fetches are meterialized immediately

dkurepa · 2025-12-15T08:59:08Z

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

@@ -0,0 +1,178 @@
+// Licensed to the .NET Foundation under one or more agreements.


should this go to the helpers folder?

I think Helper might not be the best name for this class. It does quite a lot of the core logic

To be fair, all the methods here are internal and static, so it's kind of a helper in that sense (there's zero side effects in that class).

It's hard to find another name, but maybe if i separate the converters into one class and the entity diff computations into another, we can have a converters class and a .. ingestion diff.. computer.. class

dkurepa · 2025-12-15T08:59:25Z

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

+        if (existingChannel is null)
+        {
+            //todo find the right exception type
+            throw new InvalidOperationException(


darc exception?

think we often use those when it's an internal logic exception

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataUpdate.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/SubscriptionYaml.cs

src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/DefaultChannelYaml.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/Validations/EntityValidator.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

...oConfiguration/src/Microsoft.DotNet.MaestroConfiguration.Client/Models/DefaultChannelYaml.cs

premun · 2025-12-15T16:06:25Z

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

+    /// </summary>
+    private static string SubscriptionComparisonKey(Data.Models.Subscription subscription)
+    {
+        if (subscription.SourceEnabled)


Do we need this if? Can't we just have || in the hash?

The relevant fields are a bit different for source-enabled and standard subscriptions. How do you propose to do it?

How do they differ? Only by source directory, no?

We can keep all the relevant fields and if source enabled is not equal then subscriptions are also not equal?

I think there's a bug - target repository should most likely be in the bottom hash too.

What about the channel id? I added it for dependency flows, but not for codeflows. Should the strings here not have the channel id in both cases?

Oh, I see, you've clumped together the fact that there were similar subscriptions before and the verification of different codeflows leading to the same branch+folder.
I think that's fine like this, it was just a little obscured - maybe we could have a comment about source enabled ones and when and why they are the same if they target the same dir of the same branch?

premun · 2025-12-15T16:11:41Z

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/Helpers/IExternallySyncedEntity.cs

+/// Requires a unique identifier to match existing entities with ingested ones.
+/// </summary>
+/// <typeparam name="TId">The type of the unique identifier for the entity. Must be a non-nullable type.</typeparam>
+public interface IExternallySyncedEntity<TId> where TId : notnull


Should this be internal?

This is implemented by the ingested entity POCO's that are returned by the ingestor, so has to be public

Why does it return the Ingestion POCOs and not the values from inside of them? Aren't those only here for internal comparison?

premun · 2025-12-15T16:11:56Z

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/Helpers/IngestedChannel.cs

+#nullable enable
+namespace Maestro.DataProviders.ConfigurationIngestion.Helpers;
+
+public class IngestedChannel : IExternallySyncedEntity<string>


Should these be internal?

These are used by the validation because we also check entity uniqueness and such. The validations should be public according to our plan

premun · 2025-12-15T16:12:11Z

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

+#nullable enable
+namespace Maestro.DataProviders.ConfigurationIngestion;
+
+public class ConfigurationIngestor(


Should this be internal?

It will be consumed by the CI pipeline at the config repo, so needs to be public

The interface yes, but the implementation can stay internal?
You will only get it through DI?

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

... to subscriptions string, make configuration ingestor internal

src/Maestro/Maestro.DataProviders/Properties/AssemblyInfo.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

Ah, I guess we still have stuff public

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs

src/Maestro/Maestro.DataProviders/SqlBarClient.cs

dkurepa · 2025-12-16T15:59:59Z

quick question: since the data we send to the Ingestor will be just parsed from the repo without any checks, do we have a check if the for example subscriptions are distinct, in the yaml data?

adamzip · 2025-12-16T16:02:10Z

quick question: since the data we send to the Ingestor will be just parsed from the repo without any checks, do we have a check if the for example subscriptions are distinct, in the yaml data?

Check out EntityValidator.cs - ValidateEntityUniqueness

dkurepa · 2025-12-16T16:20:39Z

Check out EntityValidator.cs - ValidateEntityUniqueness

nice, thanks!

Copilot AI review requested due to automatic review settings December 10, 2025 15:27

Copilot started reviewing on behalf of adamzip December 10, 2025 15:27 View session

adamzip force-pushed the configuration-ingestion-implementation branch from 5835a33 to 898d740 Compare December 10, 2025 15:29

Implement configuration ingestor

2407049

adamzip force-pushed the configuration-ingestion-implementation branch from 898d740 to 2407049 Compare December 10, 2025 15:32

Copilot AI reviewed Dec 10, 2025

View reviewed changes

adamzip and others added 8 commits December 10, 2025 16:38

Update src/Maestro/Maestro.DataProviders/ConfigurationIngestor/Config…

04c3c99

…urationDataHelper.cs Co-authored-by: Copilot <[email protected]>

Update src/Maestro/Maestro.DataProviders/ISqlBarClient.cs

a5c29f3

Co-authored-by: Copilot <[email protected]>

Update src/Maestro/Maestro.DataProviders/SqlBarClient.cs

58d7ae3

Co-authored-by: Copilot <[email protected]>

Update src/Maestro/Maestro.DataProviders/SqlBarClient.cs

dd64430

Co-authored-by: Copilot <[email protected]>

Update src/Microsoft.DotNet.Darc/DarcLib/Models/Yaml/IExternallySynce…

32e4cb7

…dEntity.cs Co-authored-by: Copilot <[email protected]>

Update src/Maestro/Maestro.DataProviders/SqlBarClient.cs

27603e0

Co-authored-by: Copilot <[email protected]>

apply copilot suggestion

99ecb06

ignore UniqueId field on yaml models

fdf9d75

premun reviewed Dec 10, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

src/Maestro/Maestro.DataProviders/SqlBarClient.cs Outdated Show resolved Hide resolved

adamzip commented Dec 10, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestor/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

adamzip added 2 commits December 11, 2025 20:21

Fix deletions, rename namespace, add tests

8a91cf5

Rename method because it's not actually a hash

7067d8f

adamzip commented Dec 12, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Show resolved Hide resolved

adamzip commented Dec 12, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/SqlBarClient.cs Outdated Show resolved Hide resolved

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Outdated Show resolved Hide resolved

premun reviewed Dec 12, 2025

View reviewed changes

test/Maestro.DataProviders.Tests/ConfigurationIngestionTests.cs Outdated Show resolved Hide resolved

dkurepa reviewed Dec 12, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

adamzip added 2 commits December 12, 2025 17:13

Create wrappers for uniqueId member

f9ca460

PR improvements

2afadc0

- Make some internal things public - Fix id update bug with default channels - Add AssetFilter persistence for subscriptions - Change EF queries to make sure all fetches are meterialized immediately

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataUpdate.cs Show resolved Hide resolved

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Outdated Show resolved Hide resolved

premun reviewed Dec 15, 2025

View reviewed changes

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Show resolved Hide resolved

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Show resolved Hide resolved

dkurepa reviewed Dec 15, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Show resolved Hide resolved

adamzip added 2 commits December 15, 2025 13:32

Remove wrong usage of IEnumerable

800ae0b

Add tests for excluded assets

43a32e2

dkurepa mentioned this pull request Dec 15, 2025

Make channel name immutable in darc update-channel #5659

Open

adamzip added 2 commits December 15, 2025 16:54

More CR changes

960beaf

Merge branch 'main' into configuration-ingestion-implementation

f9f79ef

premun reviewed Dec 15, 2025

View reviewed changes

force immutable name, one more test, and fix build

fea9080

dkurepa reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Show resolved Hide resolved

adamzip added 2 commits December 16, 2025 10:56

CR changes: use namespace include, add target repo ...

1909d6f

... to subscriptions string, make configuration ingestor internal

Add comment about subscriptions string

7934644

dkurepa reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/Properties/AssemblyInfo.cs Outdated Show resolved Hide resolved

adamzip added 2 commits December 16, 2025 12:34

Use hash sets instead of dictionaries in method

26e6d3b

make not async method not async

b05a405

premun reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

adamzip added 2 commits December 16, 2025 13:13

Change internalsVisibleTo

6fe0036

Use IReadOnlyCollection instead of List

45dbeef

premun previously approved these changes Dec 16, 2025

View reviewed changes

dkurepa reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationDataHelper.cs Outdated Show resolved Hide resolved

dkurepa reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/ConfigurationIngestion/ConfigurationIngestor.cs Outdated Show resolved Hide resolved

dkurepa reviewed Dec 16, 2025

View reviewed changes

src/Maestro/Maestro.DataProviders/SqlBarClient.cs Outdated Show resolved Hide resolved

improve dictionary usages

edd6b9e

dkurepa approved these changes Dec 16, 2025

View reviewed changes

		@@ -0,0 +1,178 @@
		// Licensed to the .NET Foundation under one or more agreements.

Implement configuration ingestion #5636

Are you sure you want to change the base?

Implement configuration ingestion #5636

Uh oh!

Conversation

adamzip commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamzip Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamzip Dec 15, 2025 •

edited

Loading