Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+51 −28
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec1b1dc
refactor cli and env access token implementation
theztefan Aug 8, 2025
5a71721
update releasenotes
theztefan Aug 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASENOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@

- Fixed issue where CLI commands required GH_PAT environment variable even when GitHub tokens were provided via command-line arguments (--github-source-pat, --github-target-pat). All migration commands now work correctly with CLI-only token authentication.
6 changes: 3 additions & 3 deletions src/Octoshift/Factories/GithubApiFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ GithubApi ISourceGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
{
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(throwIfNotFound: false);
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
Expand All @@ -40,7 +40,7 @@ GithubApi ISourceGithubApiFactory.CreateClientNoSsl(string apiUrl, string upload
{
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(throwIfNotFound: false);
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
Expand All @@ -50,7 +50,7 @@ GithubApi ITargetGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
{
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
targetPersonalAccessToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
targetPersonalAccessToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, targetPersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
Expand Down
2 changes: 1 addition & 1 deletion src/ado2gh/Commands/IntegrateBoards/IntegrateBoardsCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public async Task Handle(IntegrateBoardsCommandArgs args)

_log.LogInformation("Integrating Azure Boards...");

args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);

var adoTeamProjectId = await _adoApi.GetTeamProjectId(args.AdoOrg, args.AdoTeamProject);
var githubHandle = await _adoApi.GetGithubHandle(args.AdoOrg, args.AdoTeamProject, args.GithubPat);
Expand Down
2 changes: 1 addition & 1 deletion src/ado2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public async Task Handle(MigrateRepoCommandArgs args)

_log.LogInformation("Migrating Repo...");

args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);

var adoRepoUrl = GetAdoRepoUrl(args.AdoOrg, args.AdoTeamProject, args.AdoRepo, args.AdoServerUrl);

Expand Down
4 changes: 2 additions & 2 deletions src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ private async Task<string> CreateMigrationSource(MigrateRepoCommandArgs args)
{
_log.LogInformation("Creating Migration Source...");

args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);
var githubOrgId = await _githubApi.GetOrganizationId(args.GithubOrg);

try
Expand All @@ -250,7 +250,7 @@ private async Task ImportArchive(MigrateRepoCommandArgs args, string migrationSo

var bbsRepoUrl = GetBbsRepoUrl(args);

args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
args.GithubPat ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);
var githubOrgId = await _githubApi.GetOrganizationId(args.GithubOrg);

string migrationId;
Expand Down
5 changes: 1 addition & 4 deletions src/gei/Commands/MigrateCodeScanningAlerts/MigrateCodeScanningAlertsCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ public override MigrateCodeScanningAlertsCommandHandler BuildHandler(MigrateCode
throw new ArgumentNullException(nameof(sp));
}

var environmentVariableProvider = sp.GetRequiredService<EnvironmentVariableProvider>();
args.GithubSourcePat ??= environmentVariableProvider.SourceGithubPersonalAccessToken(false);
args.GithubTargetPat ??= environmentVariableProvider.TargetGithubPersonalAccessToken();

// The factory handles environment variable resolution
if (args.GithubSourcePat.IsNullOrWhiteSpace())
{
args.GithubSourcePat = args.GithubTargetPat;
Expand Down
2 changes: 1 addition & 1 deletion src/gei/Commands/MigrateOrg/MigrateOrgCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public async Task Handle(MigrateOrgCommandArgs args)
}

private string GetSourceToken(MigrateOrgCommandArgs args) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

args.GithubSourcePat ?? _environmentVariableProvider.SourceGithubPersonalAccessToken();
args.GithubSourcePat ?? _environmentVariableProvider.SourceGithubPersonalAccessToken(throwIfNotFound: false);

private string GetGithubOrgUrl(string org, string baseUrl) => $"{baseUrl ?? DEFAULT_GITHUB_BASE_URL}/{org.EscapeDataString()}";
}
4 changes: 2 additions & 2 deletions src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public async Task Handle(MigrateRepoCommandArgs args)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var targetToken = args.GithubTargetPat ?? _environmentVariableProvider.TargetGithubPersonalAccessToken(throwIfNotFound: false);

string migrationId;

Expand Down Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private string GetSourceToken(MigrateRepoCommandArgs args) => args.GithubSourcePat ?? _environmentVariableProvider.SourceGithubPersonalAccessToken(throwIfNotFound: false);

private string GetSourceRepoUrl(MigrateRepoCommandArgs args) => GetGithubRepoUrl(args.GithubSourceOrg, args.SourceRepo, args.GhesApiUrl.HasValue() ? ExtractGhesBaseUrl(args.GhesApiUrl) : null);

Expand Down
5 changes: 1 addition & 4 deletions src/gei/Commands/MigrateSecretAlerts/MigrateSecretAlertsCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ public override MigrateSecretAlertsCommandHandler BuildHandler(MigrateSecretAler
throw new ArgumentNullException(nameof(sp));
}

var environmentVariableProvider = sp.GetRequiredService<EnvironmentVariableProvider>();
args.GithubSourcePat ??= environmentVariableProvider.SourceGithubPersonalAccessToken(false);
args.GithubTargetPat ??= environmentVariableProvider.TargetGithubPersonalAccessToken();

// The factory handles environment variable resolution
if (args.GithubSourcePat.IsNullOrWhiteSpace())
{
args.GithubSourcePat = args.GithubTargetPat;
Expand Down
24 changes: 19 additions & 5 deletions src/gei/Factories/CodeScanningAlertServiceFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,27 @@ public CodeScanningAlertServiceFactory(
public virtual CodeScanningAlertService
Create(string sourceApi, string sourceToken, string targetApi, string targetToken, bool sourceApiNoSsl = false)
{
sourceToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
targetToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
// Only resolve from environment if tokens are explicitly null
// Use consistent throwIfNotFound=false to prevent exceptions when CLI args are preferred
sourceToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(false);

targetToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(false);

// Validate that we have tokens after all resolution attempts
if (string.IsNullOrWhiteSpace(sourceToken))
{
throw new OctoshiftCliException("Source GitHub Personal Access Token is required. Provide it via --github-source-pat argument or GH_SOURCE_PAT/GH_PAT environment variable.");
}

if (string.IsNullOrWhiteSpace(targetToken))
{
throw new OctoshiftCliException("Target GitHub Personal Access Token is required. Provide it via --github-target-pat argument or GH_PAT environment variable.");
}

var sourceGithubApi = sourceApiNoSsl
? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, sourceToken)
: _sourceGithubApiFactory.Create(sourceApi, sourceToken);
? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, null, sourceToken)
: _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);
Copy link
Preview

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.


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

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

Copilot uses AI. Check for mistakes.

}
}
24 changes: 19 additions & 5 deletions src/gei/Factories/SecretScanningAlertServiceFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,27 @@ public SecretScanningAlertServiceFactory(
public virtual SecretScanningAlertService
Create(string sourceApi, string sourceToken, string targetApi, string targetToken, bool sourceApiNoSsl = false)
{
sourceToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
targetToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
// Only resolve from environment if tokens are explicitly null
// Use consistent throwIfNotFound=false to prevent exceptions when CLI args are preferred
sourceToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(false);

targetToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(false);

// Validate that we have tokens after all resolution attempts
if (string.IsNullOrWhiteSpace(sourceToken))
{
throw new OctoshiftCliException("Source GitHub Personal Access Token is required. Provide it via --github-source-pat argument or GH_SOURCE_PAT/GH_PAT environment variable.");
}

if (string.IsNullOrWhiteSpace(targetToken))
{
throw new OctoshiftCliException("Target GitHub Personal Access Token is required. Provide it via --github-target-pat argument or GH_PAT environment variable.");
}

var sourceGithubApi = sourceApiNoSsl
? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, sourceToken)
: _sourceGithubApiFactory.Create(sourceApi, sourceToken);
? _sourceGithubApiFactory.CreateClientNoSsl(sourceApi, null, sourceToken)
: _sourceGithubApiFactory.Create(sourceApi, null, sourceToken);
Copy link
Preview

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.


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

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.

}
}
Loading