Add configuration repository support to `darc add-default-channel` #5684

Copilot · 2025-12-22T12:14:28Z

Co-authored-by: dkurepa <91743470+dkurepa@users.noreply.github.com>

Copilot

Pull request overview

This PR extends the darc add-default-channel command to support writing default channel configurations directly to the maestro-configuration repository, enabling a dual-mode operation during the transition from API-based to file-based configuration management. The implementation follows the established pattern from add-subscription and is controlled by the DARC_USE_CONFIGURATION_REPOSITORY environment variable.

Key Changes

Added configuration repository support to darc add-default-channel with dual-mode operation (API vs. config repo)
Implemented BAR validation before writing to config repo to prevent duplicates across both systems
Comprehensive test suite covering file creation, appending, and duplicate detection scenarios

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`src/Microsoft.DotNet.Darc/Darc/Options/AddDefaultChannelCommandLineOptions.cs`	Changed base class to `ConfigurationManagementCommandLineOptions` to inherit configuration repository flags
`src/Microsoft.DotNet.Darc/Darc/Operations/AddDefaultChannelOperation.cs`	Added dual-mode operation routing, BAR validation, and config repo integration for default channel management
`src/MaestroConfiguration/src/Microsoft.DotNet.MaestroConfiguration.Client/IConfigurationRepositoryManager.cs`	Added `AddDefaultChannelAsync` method signature to the interface
`src/MaestroConfiguration/src/Microsoft.DotNet.MaestroConfiguration.Client/ConfigurationRepositoryManager.cs`	Implemented `AddDefaultChannelAsync` with duplicate detection and proper file path resolution
`test/Microsoft.DotNet.Darc.Tests/Operations/AddDefaultChannelOperationConfigRepoTests.cs`	Added comprehensive test suite covering file creation, appending to existing files, and duplicate detection in YAML files

src/Microsoft.DotNet.Darc/Darc/Operations/AddDefaultChannelOperation.cs

Copilot · 2025-12-25T10:27:25Z

test/Microsoft.DotNet.Darc.Tests/Operations/AddDefaultChannelOperationConfigRepoTests.cs

+        var content = await File.ReadAllTextAsync(filePath);
+        return YamlDeserializer.Deserialize<List<DefaultChannelYaml>>(content) ?? [];
+    }
+}


The test suite is missing a test case for when ValidateNoEquivalentDefaultChannel fails because a default channel already exists in BAR. This is an important scenario to test since it validates the BAR-level duplicate detection logic. Consider adding a test that mocks _barClient.GetDefaultChannelsAsync to return an existing default channel, then verifies that ExecuteAsync returns Constants.ErrorCode and logs the appropriate error message.

test/Microsoft.DotNet.Darc.Tests/Operations/AddDefaultChannelOperationConfigRepoTests.cs

Copilot · 2025-12-25T10:27:26Z

src/Microsoft.DotNet.Darc/Darc/Operations/AddDefaultChannelOperation.cs

+        {
+            _logger.LogError("A default channel with the same repository, branch, and channel already exists (ID: {id})",
+                existingDefaultChannel.Id);
+            throw new ArgumentException($"A default channel with the repository {existingDefaultChannel.Repository}, branch {existingDefaultChannel.Branch} and channel {existingDefaultChannel.Channel} already exists");


Default 'ToString()': Channel inherits 'ToString()' from 'Object', and so is not suitable for printing.

Suggested change

throw new ArgumentException($"A default channel with the repository {existingDefaultChannel.Repository}, branch {existingDefaultChannel.Branch} and channel {existingDefaultChannel.Channel} already exists");

throw new ArgumentException($"A default channel with the repository {existingDefaultChannel.Repository}, branch {existingDefaultChannel.Branch} and channel {existingDefaultChannel.Channel.Name} already exists");

adamzip · 2025-12-31T15:34:34Z

...iguration/src/Microsoft.DotNet.MaestroConfiguration.Client/ConfigurationRepositoryManager.cs

+                    $"Add new default channel ({dc.Channel}) {dc.Repository} ({dc.Branch})"),
+                $"Successfully added default channel on branch '{parameters.ConfigurationBranch}' of the configuration repository {parameters.RepositoryUri}");
+        }
+        catch (DuplicateConfigurationObjectException ex)


These ones look like they should just be caught at the base operation - or even better, I think we already have a global catcher at the darc operation base which prints the exception message without the stack trace - so we don't need to catch it anywhere.

There should probably be no logging at all in this class, since it's actually a library. We don't know where this code will run.

I think we want these to do the logging because they're a library

I think we want these to do the logging because they're a library

Doing a catch and rethrow just for logging is redundant, I don't see the point. The exception goes through to the caller of the library anyway, so the caller will either catch it himself anyway, or let it make his process quit unsuccessfully

Is it though? If we have 5 places we use the library from, do all 5 now need to catch+log so that they explain what the exception means to the user?

I don't think catching and rethrowing just for a log should be done anywhere, not in the library and not by the caller.
Notice that in our darc operations we already have a catcher on the base operation level that hides the stack trace but shows the error message to the user - that's perfect usage of it IMO. There was no need to catch and rethrow just for logging anywhere inbetween.

If the exception has a similar message inside then I agree it's not worth catching+logging+rethrowing.
If this logging in the catch explains some important detail of the exception that only the client library understands then I can see it staying. Alternatively, we could catch here and rethrow with a different exception message that is more explanatory - e.g. instead of throwing "entity like this already exists in a file on branch", we'd set it to "this channel already exists" or "channel with the same name already exists" which is more user-friendly.

I made the logging only happen in the darc operations, and improved the messaging on the exceptions themselves for whoever catches it while using the library.
Let me know if you think it's good so I can update other PRs with it

adamzip · 2025-12-31T16:01:11Z

...iguration/src/Microsoft.DotNet.MaestroConfiguration.Client/ConfigurationRepositoryManager.cs

+                        && string.Equals(existing.Channel, newDc.Channel, StringComparison.OrdinalIgnoreCase),
+                    new DefaultChannelYamlComparer(),
+                    $"Add new default channel ({dc.Channel}) {dc.Repository} ({dc.Branch})"),
+                $"Successfully added default channel on branch '{parameters.ConfigurationBranch}' of the configuration repository {parameters.RepositoryUri}");


The success message seems like a darc operation concern, not something that should be in the config repo client library

Initial plan

0732dac

Copilot AI assigned Copilot and dkurepa Dec 22, 2025

Copilot started work on behalf of dkurepa December 22, 2025 12:14 View session

Add config repo support for add-default-channel command

683a1d7

Co-authored-by: dkurepa <91743470+dkurepa@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Update darc add-default-channel command for configuration repo~~ Add configuration repository support to darc add-default-channel Dec 22, 2025

Copilot AI requested a review from dkurepa December 22, 2025 12:33

Copilot finished work on behalf of dkurepa December 22, 2025 12:33

Cleanup PR

f221d32

dkurepa marked this pull request as ready for review December 25, 2025 10:20

Copilot AI review requested due to automatic review settings December 25, 2025 10:20

Copilot started reviewing on behalf of dkurepa December 25, 2025 10:20 View session

Copilot AI reviewed Dec 25, 2025

View reviewed changes

adamzip reviewed Dec 31, 2025

View reviewed changes

premun previously approved these changes Jan 5, 2026

View reviewed changes

Handle exceptions better

fa88415

dkurepa dismissed premun’s stale review via fa88415 January 5, 2026 16:31

more TODOs

bdd142f

premun approved these changes Jan 5, 2026

View reviewed changes

dkurepa merged commit 92a1274 into main Jan 5, 2026
7 of 9 checks passed

dkurepa deleted the copilot/update-darc-add-default-channel branch January 5, 2026 17:15

	throw new ArgumentException($"A default channel with the repository {existingDefaultChannel.Repository}, branch {existingDefaultChannel.Branch} and channel {existingDefaultChannel.Channel} already exists");
	throw new ArgumentException($"A default channel with the repository {existingDefaultChannel.Repository}, branch {existingDefaultChannel.Branch} and channel {existingDefaultChannel.Channel.Name} already exists");

Add configuration repository support to darc add-default-channel #5684

Add configuration repository support to darc add-default-channel #5684

Uh oh!

Conversation

Copilot AI commented Dec 22, 2025 • edited by dkurepa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 25, 2025

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!

premun Jan 5, 2026 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add configuration repository support to `darc add-default-channel` #5684

Add configuration repository support to `darc add-default-channel` #5684

Copilot AI commented Dec 22, 2025 •

edited by dkurepa

Loading

premun Jan 5, 2026 •

edited

Loading