Enable auth migration based on config refresh. #3786
Conversation
| @@ -37,6 +37,7 @@ public sealed class BrokerServer : RunnerService, IBrokerServer | |||
|
|
|||
| public async Task ConnectAsync(Uri serverUri, VssCredentials credentials) | |||
| { | |||
| Trace.Entering(); | |||
| @@ -306,6 +305,10 @@ public async Task<TaskAgentMessage> GetNextMessageAsync(CancellationToken token) | |||
| Trace.Error("Catch exception during get next message."); | |||
| Trace.Error(ex); | |||
|
|
|||
| // clear out potential message for broker migration, | |||
| // in case the exception is thrown from get message from broker-listener. | |||
| message = null; | |||
There was a problem hiding this comment.
in case we got migration message from pipelines, but fail to get message broker-listener.
we currently return the migration message to the caller which the caller don't know how to handle the migration message.
| @@ -50,7 +51,8 @@ public override void Initialize(IHostContext hostContext) | |||
| { | |||
| base.Initialize(hostContext); | |||
| _term = HostContext.GetService<ITerminal>(); | |||
| _acquireJobThrottler = HostContext.CreateService<IErrorThrottler>(); | |||
There was a problem hiding this comment.
we should always use GetService instead of CreateService for singleton service.
There was a problem hiding this comment.
This was originally intended not to be a singleton. Why switch to singleton?
There was a problem hiding this comment.
The error throttler helps us back off when encountering successive, non-retriable errors from /acquirejob.
There was a problem hiding this comment.
i didn't find any place we use IErrorThrottler outside of the Runner.cs, so i thought we only need one.
I guess, if the original idea is to create IErrorThrottler as needed, then CreateService make more sense.
There was a problem hiding this comment.
i changed it back for now.
src/Runner.Listener/Runner.cs
Outdated
| @@ -252,7 +254,7 @@ public async Task<int> ExecuteCommand(CommandSettings command) | |||
| } | |||
| } | |||
|
|
|||
| RunnerSettings settings = configManager.LoadSettings(); | |||
| _runnerSettings = configManager.LoadSettings(); | |||
There was a problem hiding this comment.
make this a global variable, so we can reuse it later (next PR) without reloading it from disk.
There was a problem hiding this comment.
nit: instance not global
There was a problem hiding this comment.
Multiple methods throughout this class receive RunnerSettings as a parameter. Will those all switch to use the instance variable instead?
There was a problem hiding this comment.
nvm, i am going to switch back, the instance var might cause more changes...
| } | ||
| else | ||
| { | ||
| HostContext.DeferAuthMigration(TimeSpan.FromDays(365), "Credential file does not contain authorizationUrlV2"); |
There was a problem hiding this comment.
this is acting as disable the migration via FF from the service.
There was a problem hiding this comment.
I'm fuzzy on this detail. This PR seems fine for now. I will catch up on this detail on the next PR.
| @@ -18,24 +18,21 @@ public sealed class BrokerMessageListenerL0 | |||
| private readonly Mock<IBrokerServer> _brokerServer; | |||
| private readonly Mock<IRunnerServer> _runnerServer; | |||
| private readonly Mock<ICredentialManager> _credMgr; | |||
| private Mock<IConfigurationStore> _store; | |||
There was a problem hiding this comment.
we don't use this any more.
| @@ -103,8 +103,8 @@ public async Task Delay(TimeSpan delay, CancellationToken token) | |||
| handler(this, new DelayEventArgs(delay, token)); | |||
| } | |||
|
|
|||
| // Delay zero | |||
| await Task.Delay(TimeSpan.Zero); | |||
There was a problem hiding this comment.
zero will make the test case stuck in case we use the HostContext.Delay in some background task.
After refresh
.credentialswith the service, we can enable or disable auth migration based on whetherauthorizationurlv2exists or not.