Remove unnecessary connection test during some registration flows#4244
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates runner registration to skip the _runnerServer.ConnectAsync(...) connectivity test when the runner-admin registration flow is in use, avoiding an extra connection attempt during that path.
Changes:
- Conditionally skip the runner-server connection validation when
UseRunnerAdminFlowis enabled. - Update in-code comments to describe why the connection test is skipped in runner-admin flow.
| // Validate can connect using the obtained vss credentials. | ||
| // In Runner Admin flow there's nothing new to test connection to at this point as registerToken is already validated via GetTenantCredential. | ||
| if (!runnerSettings.UseRunnerAdminFlow) | ||
| { | ||
| await _runnerServer.ConnectAsync(new Uri(runnerSettings.ServerUrl), creds); | ||
| } |
There was a problem hiding this comment.
When UseRunnerAdminFlow is true, this skips the connection check but still prints "Connected to GitHub" and logs "Test Connection complete." This makes the output/telemetry ambiguous because no runner-server connectivity was actually validated in this path. Consider adjusting the messaging/logging (or emitting an explicit "skipping connection test" trace) so operators can tell whether a real _runnerServer.ConnectAsync succeeded.
See below for a potential fix:
_term.WriteLine();
_term.WriteSuccessMessage("Connected to GitHub");
Trace.Info("Test Connection complete.");
}
else
{
Trace.Info("Skipping runner-server connection test because runner-admin flow has already validated the registration token.");
_term.WriteLine();
_term.WriteSuccessMessage("GitHub authentication succeeded (runner-admin flow; no additional connection test performed).");
}
| // In Runner Admin flow there's nothing new to test connection to at this point as registerToken is already validated via GetTenantCredential. | ||
| if (!runnerSettings.UseRunnerAdminFlow) | ||
| { | ||
| await _runnerServer.ConnectAsync(new Uri(runnerSettings.ServerUrl), creds); | ||
| } |
There was a problem hiding this comment.
This change introduces a new branch where _runnerServer.ConnectAsync(...) is intentionally not called for runner-admin flow. There are existing L0 tests for ConfigurationManager.ConfigureAsync, but none assert this new behavior. Please add a unit test that exercises the runner-admin flow and verifies the connection test is skipped (and, if applicable, that the flow still proceeds to runner group discovery/registration).
No description provided.