Refactor IPrintService and enhance CLI operation handling#12
Merged
Conversation
… the two Print<> overloads
- PrintService: Removed PrintHeader implementation and IDataverseReader constructor parameter
- PluginSyncService, WebresourceSyncService, IdentitySyncService: Removed IPrintService constructor parameter; opening log message now goes through each service's own ILogger<T>
- XrmSyncCommandBase.RunAction: Logs "Connected to Dataverse at {url}" via nullable GetService<IDataverseReader>() — the single centralized location
There was a problem hiding this comment.
Pull request overview
This PR refactors logging/CLI handling across sync commands (notably identity operations), removing header-printing via IPrintService in sync services, and tightening identity operation parsing/validation with accompanying test updates.
Changes:
- Replace
PrintHeaderusage in sync services with directILoggercalls and adjustPrintService/IPrintServiceaccordingly. - Allow
IdentitySyncItem.Operationto be nullable when absent from config, and enforce “operation must be supplied” via validation/CLI merge. - Update/extend tests to match the new logging and identity validation behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| XrmSync/Program.cs | Prints tool header at startup and wires CLI commands. |
| XrmSync/Options/XrmSyncConfigurationValidator.cs | Adds validation for missing identity operations; improves error context formatting. |
| XrmSync/Options/XrmSyncConfigurationBuilder.cs | Parses missing identity operation as null (invalid strings still skip the item). |
| XrmSync/Constants/CliOptions.cs | Makes identity --operation optional (arity updated). |
| XrmSync/Commands/XrmSyncRootCommand.cs | Refactors override handling (nullable bool overrides), removes header log, adds identity null-op guard. |
| XrmSync/Commands/XrmSyncCommandBase.cs | Logs Dataverse connection info via IDataverseReader before running actions. |
| XrmSync/Commands/IdentityCommand.cs | Supports nullable operation, merges root overrides, and adds explicit validation messaging. |
| SyncService/WebresourceSyncService.cs | Replaces header print with logger message. |
| SyncService/PluginSyncService.cs | Replaces header print with logger message. |
| SyncService/IdentitySyncService.cs | Replaces header print with structured logging. |
| SyncService/Difference/PrintService.cs | Removes Dataverse dependency and drops header printing behavior. |
| SyncService/Difference/IPrintService.cs | Removes PrintHeader API and PrintHeaderOptions. |
| Model/XrmSyncOptions.cs | Makes IdentitySyncItem.Operation nullable and adjusts SyncType/Empty. |
| Tests/* | Updates tests for removed IPrintService usage and adds identity null-operation validation coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…umentArity.ZeroOrOne to both DryRun and CiMode descriptors. This allows them to work both as bare flags (--dry-run) and with explicit values (--dry-run false), and ensures
GetValue() returns null (not false) when absent.
2. Unused description variable (XrmSyncRootCommand.cs) Removed the IDescription DI registration and the description local variable — both became dead code after moving the header to Program.cs.
3. Sync-item selection predicate (IdentityCommand.cs:106-110) Replaced the single .FirstOrDefault predicate with explicit priority ordering:
- When --operation is given: exact match first, null-operation item as fallback - When --operation is absent: first identity item (any)
This prevents a null-operation item earlier in the list from shadowing an exact-match item.
4. Tool header pollution (Program.cs)
Moved Console.WriteLine(new Description().ToolHeader) to after parsing, guarded by a check for --help/-h/-?/--version. The header is now suppressed for help and version requests but still
prints for all actual command executions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ard in the switch expression catches it early where logger is in scope, calling LogMissingIdentityOperation(logger). The defensive Console.Error.WriteLine inside ExecuteIdentity is gone; the method now only runs with a guaranteed non-null operation. 2. Tool header stdout pollution — changed Console.WriteLine → Console.Error.WriteLine. The banner goes to stderr, leaving stdout clean for analyze's JSON and any other machine-readable output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors how logging and option handling works for identity, plugin, and webresource sync services, and improves validation and test coverage for identity operations. The main changes involve removing the
IPrintServiceabstraction in favor of direct logging, updating how identity operations are handled and validated, and improving test cases to reflect these changes.Logging and Service Refactoring:
IPrintServiceinterface and its usages from all sync services (IdentitySyncService,PluginSyncService,WebresourceSyncService), replacing calls toPrintHeaderwith directILoggercalls for better simplicity and maintainability. [1] [2] [3] [4] [5] [6]Identity Operation Handling and Validation:
IdentitySyncItemto allow a nullableOperationand improved its default handling and string representation. Now, missing operations in configuration are parsed asnull, and validation ensures an operation is eventually provided (e.g., via CLI). [1] [2] [3] [4]IdentityCommandto handle nullable operations, merge CLI and config values correctly, and provide clear validation errors if the operation is missing. [1] [2] [3] [4]Test Updates:
IPrintService, and added/adjusted tests to verify the new identity operation parsing and validation logic. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]These changes simplify the codebase, make logging more consistent, and ensure that identity operations are validated robustly, improving both maintainability and user experience.