Allow enable auth migration by default. #3804
Open
Conversation
This file contains 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
There was a problem hiding this comment.
Pull Request Overview
This PR enables authentication migration by default when the appropriate configuration is set.
- Added a new unit test in RunnerL0.cs to validate the default activation of auth migration.
- Updated the ExecuteCommand logic in Runner.Listener/Runner.cs to trigger auth migration based on credentials.
- Modified the log message in HostContext.cs to reflect the time of auth migration using the current UTC time.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Test/L0/Listener/RunnerL0.cs | Added unit test for verifying auth migration behavior. |
| src/Runner.Listener/Runner.cs | Updated command execution logic to check credentials and enable auth migration. |
| src/Runner.Common/HostContext.cs | Changed log message to display the current UTC time instead of a deferred timestamp. |
Comments suppressed due to low confidence (2)
src/Runner.Listener/Runner.cs:318
- [nitpick] Consider defining a constant for the 'EnableAuthMigrationByDefault' flag to prevent potential typos and ensure consistency.
HostContext.EnableAuthMigration("EnableAuthMigrationByDefault");
src/Runner.Common/HostContext.cs:271
- [nitpick] The log message now uses the current UTC time instead of the deferred auth migration time, which may affect debugging; please verify that this change is intentional.
_trace.Info($"Enable auth migration at {DateTime.UtcNow.ToString("O")}");
TingluoHuang
commented
Apr 10, 2025
| @@ -268,7 +268,7 @@ public void EnableAuthMigration(string trace) | |||
| } | |||
| } | |||
|
|
|||
| _trace.Info($"Enable auth migration at {_deferredAuthMigrationTime.ToString("O")}."); | |||
There was a problem hiding this comment.
copilot used the wrong time before... _deferredAuthMigrationTime is a time in the future, i want to log the time at this point.
TingluoHuang
commented
Apr 10, 2025
| var cred = store.GetCredentials(); | ||
| if (cred != null && | ||
| cred.Scheme == Constants.Configuration.OAuth && | ||
| cred.Data.ContainsKey("EnableAuthMigrationByDefault")) |
There was a problem hiding this comment.
EnableAuthMigrationByDefault in the .credentials will make the runner to try migration without waiting for service.
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.
This pull request introduces changes to enable authentication migration by default when certain conditions are met. The most important changes include modifications to the
EnableAuthMigrationmethod, updates to theExecuteCommandmethod, and the addition of a new unit test to verify the new functionality.