chore: executor framework admin actions code#8093
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements instance management functionality in the CloudClientExecutor, including update, delete, list, and get operations. Review feedback identifies a missing import for status, a logic error in the executeUpdateCloudInstance method where displayName was incorrectly assigned, and the hardcoding of nextPageToken which prevents proper pagination in executeListCloudInstances.
| import * as protos from '../../protos/protos'; | ||
| import {CloudUtil} from './cloud-util'; | ||
| import {OutcomeSender, ExecutionFlowContextInterface} from './cloud-executor'; | ||
| import {OutcomeSender, ExecutionFlowContextInterface, CloudExecutor} from './cloud-executor'; |
There was a problem hiding this comment.
The code uses status.OK in several methods (lines 374, 409), but status is not imported in this file. It should be imported from @grpc/grpc-js to avoid a ReferenceError.
| import {OutcomeSender, ExecutionFlowContextInterface, CloudExecutor} from './cloud-executor'; | |
| import {status} from '@grpc/grpc-js'; | |
| import {OutcomeSender, ExecutionFlowContextInterface, CloudExecutor} from './cloud-executor'; |
| displayName: | ||
| action.displayName !== undefined ? instanceId : undefined, |
There was a problem hiding this comment.
The displayName is incorrectly being set to instanceId when action.displayName is provided. It should be set to the value of action.displayName.
displayName: action.displayName,| const [instances] = await instanceAdminClient.listInstances({ | ||
| parent: instanceAdminClient.projectPath(projectId), | ||
| filter: action.filter, | ||
| pageSize: action.pageSize, | ||
| pageToken: action.pageToken, | ||
| }); | ||
|
|
||
| console.log(`Found ${instances.length} instances.`); | ||
|
|
||
| const outcome = SpannerActionOutcome.create({ | ||
| status: CloudExecutor.toProto(status.OK), | ||
| adminResult: { | ||
| instanceResponse: { | ||
| listedInstances: instances, | ||
| nextPageToken: '', | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The nextPageToken from the listInstances response is currently ignored and hardcoded to an empty string, which breaks pagination for the executor framework.
| const [instances] = await instanceAdminClient.listInstances({ | |
| parent: instanceAdminClient.projectPath(projectId), | |
| filter: action.filter, | |
| pageSize: action.pageSize, | |
| pageToken: action.pageToken, | |
| }); | |
| console.log(`Found ${instances.length} instances.`); | |
| const outcome = SpannerActionOutcome.create({ | |
| status: CloudExecutor.toProto(status.OK), | |
| adminResult: { | |
| instanceResponse: { | |
| listedInstances: instances, | |
| nextPageToken: '', | |
| }, | |
| }, | |
| }); | |
| const [instances, , response] = await instanceAdminClient.listInstances({ | |
| parent: instanceAdminClient.projectPath(projectId), | |
| filter: action.filter, | |
| pageSize: action.pageSize, | |
| pageToken: action.pageToken, | |
| }); | |
| console.log(`Found ${instances.length} instances.`); | |
| const outcome = SpannerActionOutcome.create({ | |
| status: CloudExecutor.toProto(status.OK), | |
| adminResult: { | |
| instanceResponse: { | |
| listedInstances: instances, | |
| nextPageToken: response?.nextPageToken || '', | |
| }, | |
| }, | |
| }); |
1 participant
No description provided.