Refactor the inconsistent access token resolution between ENV and CLI args #1410

theztefan · 2025-08-08T14:58:07Z

This pull request improves how GitHub Personal Access Tokens (PATs) are resolved throughout the codebase.

The issue was identified in the #1409. After investigating it turned out there were inconsistencies on how we resolve PATs CLI provided and ENV provided ones.

The following issues were identified:

Redundant token resolution: Both command handlers and factories attempt to resolve tokens from environment variables
Inconsistent error handling: Different throwIfNotFound parameters across the codebase
Tight coupling: Factories are tightly coupled to EnvironmentVariableProvider even when not needed
Pattern inconsistency: Different factory classes may have different token resolution behaviors

This PR tries to consolidate to one solution and uses the same pattern everywhere. The order is as follows:

CLI args first: --github-source-pat, --github-target-pat, etc.
ENV variables second: GH_SOURCE_PAT, GH_PAT, etc.
Gracefull fallback: No exceptions thrown if env vars missing
Clear error messages: Mode descriptitive errors only when no token is found at all

Improvements from user perspective:

CLI arguments work reliably without requiring environment variables
Consistent behavior across all migration commands

Code perspective:

Clean separation of concerns between commands and factories
Consistent error handling
Single source of truth for token resolution logic
Easier to add new commands with token requirements

Did you write/update appropriate tests
Release notes updated (if appropriate)
Appropriate logging output
Issue linked
Docs updated (or issue created)
New package licenses are added to ThirdPartyNotices.txt (if applicable)

Copilot

Pull Request Overview

This pull request standardizes GitHub Personal Access Token (PAT) resolution across all CLI commands by refactoring the inconsistent handling between command-line arguments and environment variables. The primary goal is to ensure CLI-provided tokens take precedence without requiring environment variables to be set.

Key changes include:

Unified token resolution strategy with CLI args taking precedence over environment variables
Consistent error handling with throwIfNotFound=false for environment variable resolution
Centralized validation in factory classes with descriptive error messages

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/gei/Factories/SecretScanningAlertServiceFactory.cs`	Added consistent token resolution with fallback and validation logic
`src/gei/Factories/CodeScanningAlertServiceFactory.cs`	Added consistent token resolution with fallback and validation logic
`src/gei/Commands/MigrateSecretAlerts/MigrateSecretAlertsCommand.cs`	Removed redundant environment variable resolution from command handler
`src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`src/gei/Commands/MigrateOrg/MigrateOrgCommandHandler.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`src/gei/Commands/MigrateCodeScanningAlerts/MigrateCodeScanningAlertsCommand.cs`	Removed redundant environment variable resolution from command handler
`src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`src/ado2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`src/ado2gh/Commands/IntegrateBoards/IntegrateBoardsCommandHandler.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`src/Octoshift/Factories/GithubApiFactory.cs`	Updated to use `throwIfNotFound=false` for environment variable resolution
`RELEASENOTES.md`	Added user-friendly description of the fix

Copilot · 2025-08-08T14:59:03Z

src/gei/Factories/SecretScanningAlertServiceFactory.cs

-            ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, sourceToken)
-            : _sourceGithubApiFactory.Create(sourceApi, sourceToken);
+            ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, null, sourceToken)
+            : _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);


[nitpick] The null parameter being passed as the second argument appears to be for uploadsUrl. Consider using a named parameter or constant to make the intent clearer, such as uploadsUrl: null or defining a constant for the default uploads URL.

Suggested change

: _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);

: _sourceGithubApiFactory.Create(sourceApi, uploadsUrl: null, sourceToken);

Copilot · 2025-08-08T14:59:04Z

src/gei/Factories/SecretScanningAlertServiceFactory.cs


-        return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, targetToken), _octoLogger);
+        return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, null, targetToken), _octoLogger);


[nitpick] The null parameter being passed as the second argument appears to be for uploadsUrl. Consider using a named parameter or constant to make the intent clearer, such as uploadsUrl: null or defining a constant for the default uploads URL.

Note: See the diff below for a potential fix:

@@ -43,9 +43,9 @@ } var sourceGithubApi = sourceApiNoSsl - ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, null, sourceToken) - : _sourceGithubApiFactory.Create(sourceApi, null, sourceToken); + ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, uploadsUrl: null, sourceToken) + : _sourceGithubApiFactory.Create(sourceApi, uploadsUrl: null, sourceToken); - return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, null, targetToken), _octoLogger); + return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, uploadsUrl: null, targetToken), _octoLogger); } }

Copilot · 2025-08-08T14:59:04Z

src/gei/Factories/CodeScanningAlertServiceFactory.cs

-            ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, sourceToken)
-            : _sourceGithubApiFactory.Create(sourceApi, sourceToken);
+            ? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, null, sourceToken)
+            : _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);


[nitpick] The null parameter being passed as the second argument appears to be for uploadsUrl. Consider using a named parameter or constant to make the intent clearer, such as uploadsUrl: null or defining a constant for the default uploads URL.

Suggested change

: _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);

: _sourceGithubApiFactory.Create(sourceApi, uploadsUrl: null, sourceToken);

Copilot · 2025-08-08T14:59:05Z

src/gei/Factories/CodeScanningAlertServiceFactory.cs


-        return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, targetToken), _octoLogger);
+        return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, null, targetToken), _octoLogger);


[nitpick] The null parameter being passed as the second argument appears to be for uploadsUrl. Consider using a named parameter or constant to make the intent clearer, such as uploadsUrl: null or defining a constant for the default uploads URL.

Suggested change

return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, null, targetToken), _octoLogger);

return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, uploadsUrl: null, targetToken), _octoLogger);

github-actions · 2025-08-08T15:03:08Z

Unit Test Results

1 files 1 suites 20s ⏱️
901 tests 901 ✅ 0 💤 0 ❌
902 runs 902 ✅ 0 💤 0 ❌

Results for commit 5a71721.

github-actions · 2025-08-08T15:03:09Z

Package	Line Rate	Branch Rate	Complexity	Health
gei	80%	71%	574	✔
bbs2gh	83%	77%	653	✔
ado2gh	83%	77%	613	✔
Octoshift	87%	76%	1479	✔
Summary	85% (7275 / 8595)	76% (1706 / 2252)	3319	✔

dylan-smith · 2025-08-08T15:24:21Z

src/gei/Commands/MigrateOrg/MigrateOrgCommandHandler.cs

@@ -72,7 +72,7 @@ public async Task Handle(MigrateOrgCommandArgs args)
    }

    private string GetSourceToken(MigrateOrgCommandArgs args) =>


Don't we want to throw here? If we can't find a source PAT we can't continue with this command.

dylan-smith · 2025-08-08T15:25:12Z

src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

@@ -151,7 +151,7 @@ public async Task Handle(MigrateRepoCommandArgs args)

        var sourceRepoUrl = GetSourceRepoUrl(args);
        var sourceToken = GetSourceToken(args);
-        var targetToken = args.GithubTargetPat ?? _environmentVariableProvider.TargetGithubPersonalAccessToken();


Don't we want to throw here? If we can't find a target PAT we can't continue with this command.

dylan-smith · 2025-08-08T15:27:02Z

src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

@@ -212,7 +212,7 @@ public async Task Handle(MigrateRepoCommandArgs args)
        _log.LogInformation($"Migration log available at {migrationLogUrl} or by running `gh {CliContext.RootCommand} download-logs --github-target-org {args.GithubTargetOrg} --target-repo {args.TargetRepo}`");
    }

-    private string GetSourceToken(MigrateRepoCommandArgs args) => args.GithubSourcePat ?? _environmentVariableProvider.SourceGithubPersonalAccessToken();


Don't we want to throw here? If we can't find a source PAT we can't continue with this command.

theztefan added 2 commits August 8, 2025 16:41

refactor cli and env access token implementation

ec1b1dc

update releasenotes

5a71721

Copilot AI review requested due to automatic review settings August 8, 2025 14:58

Copilot AI reviewed Aug 8, 2025

View reviewed changes

dylan-smith reviewed Aug 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor the inconsistent access token resolution between ENV and CLI args #1410

Refactor the inconsistent access token resolution between ENV and CLI args #1410

theztefan commented Aug 8, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 8, 2025

Uh oh!

Copilot AI Aug 8, 2025

Uh oh!

Copilot AI Aug 8, 2025

Uh oh!

Copilot AI Aug 8, 2025

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

dylan-smith Aug 8, 2025

Uh oh!

dylan-smith Aug 8, 2025

Uh oh!

dylan-smith Aug 8, 2025

Uh oh!

Uh oh!

	: _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);
	: _sourceGithubApiFactory.Create(sourceApi, uploadsUrl: null, sourceToken);


		return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, targetToken), _octoLogger);
		return new(sourceGithubApi, _targetGithubApiFactory.Create(targetApi, null, targetToken), _octoLogger);

		@@ -72,7 +72,7 @@ public async Task Handle(MigrateOrgCommandArgs args)
		}

		private string GetSourceToken(MigrateOrgCommandArgs args) =>

Refactor the inconsistent access token resolution between ENV and CLI args #1410

Are you sure you want to change the base?

Refactor the inconsistent access token resolution between ENV and CLI args #1410

Conversation

theztefan commented Aug 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 8, 2025

Unit Test Results

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

dylan-smith Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

dylan-smith Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

dylan-smith Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!