Make darc update-subscription use the config repo
#5665
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for updating subscriptions through the configuration repository (YAML-based approach) in the darc update-subscription command, addressing issue #5512. Previously, subscription updates only went through the BAR API client; now they can also be routed through the configuration repository when the appropriate flag is set.
Key Changes:
- Added configuration repository support to
UpdateSubscriptionOperationwith conditional routing based onShouldUseConfigurationRepository - Refactored duplicate subscription validation logic into a shared base class method
- Implemented the
UpdateSubscriptionAsyncmethod inConfigurationRepositoryManager(was previously throwingNotImplementedException)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| UpdateSubscriptionOperation.cs | Added config repo manager dependency and conditional logic to route updates through config repo or BAR API based on options |
| SubscriptionOperationBase.cs | Added config repo manager field to base class and extracted duplicate validation logic into shared method |
| AddSubscriptionOperation.cs | Refactored to use shared validation method and config repo manager from base class |
| ConfigurationRepositoryManager.cs | Implemented UpdateSubscriptionAsync method and changed DeleteSubscriptionAsync to throw exception instead of logging warning |
| triggerAutomatically = UxHelpers.PromptForYesNo("Trigger this subscription immediately?"); | ||
| } | ||
| Id = subscription.Id, | ||
| Enabled = subscriptionToUpdate.Enabled ?? false, |
There was a problem hiding this comment.
The fallback to false may incorrectly disable a subscription if subscriptionToUpdate.Enabled is null but the original subscription was enabled. Consider using subscriptionToUpdate.Enabled ?? subscription.Enabled to preserve the existing state when no update is specified.
| Enabled = subscriptionToUpdate.Enabled ?? false, | |
| Enabled = subscriptionToUpdate.Enabled ?? subscription.Enabled, |
| subscriptionsInFile.Remove(existingSubscription); | ||
| subscriptionsInFile.Add(updatedSubscription); |
There was a problem hiding this comment.
Rather than removing and adding, consider replacing the subscription in place to maintain the original list order. This can be done by finding the index and replacing at that position: var index = subscriptionsInFile.IndexOf(existingSubscription); subscriptionsInFile[index] = updatedSubscription;
| subscriptionsInFile.Remove(existingSubscription); | |
| subscriptionsInFile.Add(updatedSubscription); | |
| var index = subscriptionsInFile.IndexOf(existingSubscription); | |
| subscriptionsInFile[index] = updatedSubscription; |
| targetDirectory: subscriptionYaml.TargetDirectory)) | ||
| .FirstOrDefault(s => s.TargetBranch == subscriptionYaml.TargetBranch); | ||
|
|
||
| if (equivalentSub?.Id != null && equivalentSub.Id != subscriptionYaml.Id) |
There was a problem hiding this comment.
The null check equivalentSub?.Id != null is redundant since FirstOrDefault would return null if no subscription is found, and you can check equivalentSub != null directly. Simplify to: if (equivalentSub != null && equivalentSub.Id != subscriptionYaml.Id)
| if (equivalentSub?.Id != null && equivalentSub.Id != subscriptionYaml.Id) | |
| if (equivalentSub != null && equivalentSub.Id != subscriptionYaml.Id) |
1 participant
#5512