External PR 1410: Refactor the inconsistent access token resolution between ENV and CLI args #1481
Conversation
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
6 tasks
Unit Test Results 1 files 1 suites 10m 25s ⏱️ Results for commit 75e8af6. ♻️ This comment has been updated with latest results. |
…gh-gei into brianaj/external-pr-1410 # Conflicts: # RELEASENOTES.md
- Add test Should_Pass_Source_Pat_From_Cli_Args_To_Factory to verify source PAT from CLI args is passed to factory - Add test Should_Pass_Target_Pat_From_Cli_Args_To_Factory to verify target PAT from CLI args is passed to factory - Tests prove that CLI args are properly passed through to the factory, demonstrating the bug fix works
brianaj
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in alert migration commands where tokens provided via CLI arguments were being ignored when environment variables were not set. The fix refactors token resolution to be handled consistently by the factory classes, with proper fallback logic from CLI args → environment variables → validation errors.
Key changes:
- Moved token resolution and validation logic from command classes into factory classes
- Changed environment variable resolution to use
throwIfNotFound=falseto allow CLI args to take precedence - Added explicit validation in factories with user-friendly error messages
- Added tests to verify CLI arguments are properly passed to factories
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gei/Factories/SecretScanningAlertServiceFactory.cs | Added token resolution from environment variables (with throwIfNotFound=false), token validation with user-friendly errors, and explicit null parameter for uploadsUrl when calling GithubApiFactory |
| src/gei/Factories/CodeScanningAlertServiceFactory.cs | Added token resolution from environment variables (with throwIfNotFound=false), token validation with user-friendly errors, and explicit null parameter for uploadsUrl when calling GithubApiFactory |
| src/gei/Commands/MigrateSecretAlerts/MigrateSecretAlertsCommand.cs | Removed environment variable resolution logic, delegating it to the factory |
| src/gei/Commands/MigrateCodeScanningAlerts/MigrateCodeScanningAlertsCommand.cs | Removed environment variable resolution logic, delegating it to the factory |
| src/OctoshiftCLI.Tests/gei/Commands/MigrateSecretAlerts/MigrateSecretAlertsCommandTests.cs | Added tests to verify CLI arguments are passed correctly to factory |
| src/OctoshiftCLI.Tests/gei/Commands/MigrateCodeScanningAlerts/MigrateCodeScanningAlertsCommandTests.cs | Added tests to verify CLI arguments are passed correctly to factory |
| RELEASENOTES.md | Added user-facing description of the bug fix |
src/OctoshiftCLI.Tests/gei/Commands/MigrateSecretAlerts/MigrateSecretAlertsCommandTests.cs
Outdated
Show resolved
Hide resolved
...iftCLI.Tests/gei/Commands/MigrateCodeScanningAlerts/MigrateCodeScanningAlertsCommandTests.cs
Outdated
Show resolved
Hide resolved
Renamed Source_Pat_Should_Default_To_Target_Pat to Should_Pass_Target_Pat_From_Cli_Args_To_Factory for clarity - the new name better describes what the test is verifying.
|
mulana
approved these changes
Dec 9, 2025
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.
External PR from #1410 by @theztefan
This is based on external PR but scoped down to only the alert migration commands that were specifically affected by issue #1409. The original PR included changes to repository and organization migration commands, but those changes were incomplete (missing validation logic). To ship this fix safely, I've limited the scope to only the commands where the fix is complete.