Add V2 flow for runner deletion #3954
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a V2 flow for runner deletion by adding methods to handle runner deletion through the monolith rather than directly. The changes introduce a new deletion API method and refactor URL construction logic for better code reuse.
- Adds
DeleteRunnerAsyncmethod to support V2 runner deletion flow - Refactors URL construction logic into a reusable
GetEntityUrlhelper method - Updates existing methods to use the new helper for consistent URL generation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Runner.Listener/Configuration/ConfigurationManager.cs | Adds V2 flow logic for runner configuration with conditional branching |
| src/Runner.Common/RunnerDotcomServer.cs | Implements DeleteRunnerAsync method and refactors URL construction into GetEntityUrl helper |
|
|
||
| if (runnerSettings.UseV2Flow) | ||
| { | ||
| await _dotcomServer. |
There was a problem hiding this comment.
This line appears to be incomplete - it has an incomplete method call with no method name after '_dotcomServer.'
| await _dotcomServer. |
Copilot uses AI. Check for mistakes.
| @@ -204,6 +103,12 @@ public async Task<List<TaskAgentPool>> GetRunnerGroupsAsync(string githubUrl, st | |||
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Post, 3, "Failed to add agent", body); | |||
| } | |||
|
|
|||
| private async Task<DistributedTask.WebApi.Runner> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) | |||
There was a problem hiding this comment.
The DeleteRunnerAsync method is declared as private but should be public to match the interface declaration on line 22. This will cause a compilation error.
| private async Task<DistributedTask.WebApi.Runner> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) | |
| public async Task<DistributedTask.WebApi.Runner> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) |
Copilot uses AI. Check for mistakes.
| private async Task<DistributedTask.WebApi.Runner> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) | ||
| { | ||
| var githubApiUrl = $"{GetEntityUrl(githubUrl)}/runners/{runnerId}"; | ||
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to add agent"); |
There was a problem hiding this comment.
The error message 'Failed to add agent' is incorrect for a delete operation. It should be 'Failed to delete runner' or similar.
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to add agent"); | |
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to delete runner"); |
Copilot uses AI. Check for mistakes.
| private async Task<DistributedTask.WebApi.Runner> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) | ||
| { | ||
| var githubApiUrl = $"{GetEntityUrl(githubUrl)}/runners/{runnerId}"; | ||
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to add agent"); |
There was a problem hiding this comment.
[nitpick] Delete operations typically don't return the deleted entity. Consider changing the return type to Task or Task to indicate success/failure instead of returning the Runner object.
| return await RetryRequest<DistributedTask.WebApi.Runner>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to add agent"); | |
| private async Task<bool> DeleteRunnerAsync(string githubUrl, string githubToken, ulong runnerId) | |
| { | |
| var githubApiUrl = $"{GetEntityUrl(githubUrl)}/runners/{runnerId}"; | |
| try | |
| { | |
| await RetryRequest<object>(githubApiUrl, githubToken, RequestType.Delete, 3, "Failed to delete runner"); | |
| return true; | |
| } | |
| catch | |
| { | |
| return false; | |
| } |
Copilot uses AI. Check for mistakes.
This PR implements the V2 flow for runner deletion (going through the monolith)