Don't allow updating the batchable prop in darc, fix subscription creation comparison#6131
Don't allow updating the batchable prop in darc, fix subscription creation comparison#6131dkurepa wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses subscription batchability immutability problems by preventing invalid batchability updates from being performed via DARC and by fixing configuration ingestion’s subscription-equivalence conflict detection so “delete + recreate” flows work correctly.
Changes:
- Add a configuration ingestion test verifying that replacing a subscription with different batchability is handled as removal + creation (not an in-place update).
- Update DARC’s interactive update flow to treat
Batchableas immutable. - Adjust BAR subscription creation conflict detection to compare against EF-tracked (local) subscriptions rather than querying all subscriptions from the database.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/Maestro/Maestro.DataProviders.Tests/ConfigurationIngestionTests.cs | Adds/adjusts tests to cover batchability replacement behavior during ingestion. |
| src/Microsoft.DotNet.Darc/Darc/Operations/UpdateSubscriptionOperation.cs | Adds an immutability check to block changing Batchable during popup-based updates. |
| src/Maestro/Maestro.DataProviders/SqlBarClient.cs | Changes subscription equivalence comparison source to EF local tracked set to avoid false conflicts during ingestion delete+create. |
| if (updateSubscriptionPopUp.Batchable != subscription.Policy.Batchable) | ||
| { | ||
| immutableFieldErrors.Add($"Batchable (cannot be changed from '{subscription.Policy.Batchable}')"); | ||
| } |
There was a problem hiding this comment.
The new immutability check for Batchable is only applied when updating via the popup flow. When updating via command line options, _options.Batchable can still change the batchable value and will write an updated YAML with a different Batchable, which will later fail ingestion because BAR treats batchability as immutable. Consider rejecting --batchable on update (or at least rejecting it when it differs from the existing value) for the command-line path as well, to match the intended behavior.
| if (updateSubscriptionPopUp.Batchable != subscription.Policy.Batchable) | ||
| { | ||
| immutableFieldErrors.Add($"Batchable (cannot be changed from '{subscription.Policy.Batchable}')"); | ||
| } |
There was a problem hiding this comment.
There are existing unit tests for UpdateSubscriptionOperation (config repo tests), but none appear to cover the newly introduced behavior that Batchable is immutable during update. Adding a test that attempts to update with batchable: true/false and asserts the operation fails (and does not modify the YAML) would prevent regressions.
#6009 (comment)