-
Notifications
You must be signed in to change notification settings - Fork 81
Prevent wrong codeflow subscription configurations #5205
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
Conversation
…gurations Co-authored-by: premun <[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.
@copilot deduplicate the newly added logic so that it's re-used across darc and the controllers
I've deduplicated the validation logic as requested. Created a shared Changes in commit 092a101:
|
092a101
to
3aeb2dc
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.
@copilot Can you create a common abstract parent for AddSubscriptionOperation.cs
and UpdateSubscriptionOperation.cs
and put the ValidateCodeflowSubscriptionConflicts
there to deduplicate it between these two classes?
…scription operations Co-authored-by: premun <[email protected]>
Created Changes in commit 03f97b0. |
…ptionsAsync Co-authored-by: premun <[email protected]>
...ervice/ProductConstructionService.Api/Api/v2020_02_20/Controllers/SubscriptionsController.cs
Show resolved
Hide resolved
Co-authored-by: premun <[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
This PR implements validation to prevent conflicting VMR codeflow subscriptions and improves the API design by unifying subscription querying methods. The solution adds comprehensive validation to detect and prevent two types of conflicts: backflow conflicts (multiple VMR→repository subscriptions targeting the same repository branch) and forward flow conflicts (multiple repository→VMR subscriptions flowing into the same VMR branch and target directory).
- Server-side and client-side validation through a shared service architecture
- Unified API with enhanced
GetSubscriptionsAsync
method accepting additional filtering parameters - Clear error messages and consistent validation behavior across all entry points
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/SubscriptionsController.cs |
Adds server-side validation logic for detecting codeflow subscription conflicts in Create and UpdateSubscription endpoints |
src/Microsoft.DotNet.Darc/DarcLib/IBasicBarClient.cs |
Extends GetSubscriptionsAsync interface with additional filtering parameters for sourceEnabled, sourceDirectory, and targetDirectory |
src/Microsoft.DotNet.Darc/DarcLib/BarApiClient.cs |
Implements the enhanced GetSubscriptionsAsync method with new filtering parameters |
src/Microsoft.DotNet.Darc/Darc/Operations/SubscriptionOperationBase.cs |
New base class containing shared validation logic for subscription operations |
src/Microsoft.DotNet.Darc/Darc/Operations/AddSubscriptionOperation.cs |
Refactored to inherit from SubscriptionOperationBase and includes client-side validation |
src/Microsoft.DotNet.Darc/Darc/Operations/UpdateSubscriptionOperation.cs |
Refactored to inherit from SubscriptionOperationBase and includes client-side validation |
src/Maestro/Maestro.DataProviders/SqlBarClient.cs |
Updates data provider to support new filtering parameters in GetSubscriptionsAsync |
test/Microsoft.DotNet.Darc.Tests/Operations/GetSubscriptionsOperationTests.cs |
Updates test mocks to accommodate the new GetSubscriptionsAsync signature |
test/Microsoft.DotNet.Darc.Tests/Operations/GetBuildOperationTests.cs |
Updates test mocks to accommodate the new GetSubscriptionsAsync signature |
...ervice/ProductConstructionService.Api/Api/v2020_02_20/Controllers/SubscriptionsController.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/AddSubscriptionOperation.cs
Outdated
Show resolved
Hide resolved
...ervice/ProductConstructionService.Api/Api/v2020_02_20/Controllers/SubscriptionsController.cs
Show resolved
Hide resolved
…flicts call Co-authored-by: dkurepa <[email protected]>
Co-authored-by: dkurepa <[email protected]>
This PR implements validation to prevent misconfigured VMR codeflow subscriptions that could lead to conflicts and operational issues, while also improving the API design by unifying subscription querying methods.
Problem
Currently, it's possible to create conflicting codeflow subscriptions that can cause problems:
These misconfigurations can lead to race conditions, merge conflicts, and unexpected behavior in the dependency flow system.
Solution
Comprehensive Validation
Added validation with a shared service architecture:
ICodeflowSubscriptionValidationService
: Common interface inMaestro.Common
CodeflowSubscriptionValidationService
: Single implementation containing all validation logicCreate()
andUpdateSubscription()
AddSubscriptionOperation
andUpdateSubscriptionOperation
SubscriptionOperationBase
eliminates duplication between Add/Update operationsValidation Rules
Backflow subscriptions (VMR → repository,
sourceDirectory
set):Forward flow subscriptions (repository → VMR,
targetDirectory
set):API Improvement
Replaced the specific
GetCodeflowSubscriptionsAsync
method with a more genericGetSubscriptionsAsync
that accepts additional optional parameters:This provides a cleaner, more flexible API that supports both general subscription filtering and specific codeflow validation needs without code duplication.
Error Messages
Clear, actionable error messages help users understand and resolve conflicts:
Benefits
Fixes #5204.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.