[OTE-881] propagate bazooka failure in auxo #2537
Conversation
WalkthroughThe changes in this pull request include the introduction of a new test suite for the Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jerryfan01234
force-pushed
the
auxo-bazooka-error
branch
from
3dd45d7 to
45b90f8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
indexer/services/auxo/__tests__/index.test.ts (5)
11-20: Consider adding type safety to the Lambda client mock.While the mock setup is functionally correct, it could benefit from explicit typing to prevent potential type-related issues.
jest.mock('@aws-sdk/client-lambda', () => { const originalModule = jest.requireActual('@aws-sdk/client-lambda'); return { ...originalModule, - LambdaClient: jest.fn(() => ({ + LambdaClient: jest.fn().mockImplementation(() => ({ send: mockClientSend, - })), + })) as jest.MockedClass<typeof originalModule.LambdaClient>, InvokeCommand: originalModule.InvokeCommand, }; });
23-34: Enhance logger mock coverage.The logger mock is missing some common methods, and there's no verification of mock calls in the test.
jest.mock('@dydxprotocol-indexer/base', () => { const actualModule = jest.requireActual('@dydxprotocol-indexer/base'); return { ...actualModule, logger: { info: jest.fn(), error: jest.fn(), crit: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), }, startBugsnag: jest.fn(), }; });Consider adding expectations to verify logger calls in your test case:
expect(logger.error).toHaveBeenCalledWith( expect.stringContaining('Bazooka Lambda error'), expect.any(Error) );
37-40: Improve helper mock implementation.The current mock setup for
upgradeBazookacould be enhanced for better test coverage and type safety.jest.mock('../src/helpers', () => ({ ...jest.requireActual('../src/helpers'), - upgradeBazooka: jest.fn().mockResolvedValue(undefined), + upgradeBazooka: jest.fn().mockImplementation(async () => { + // Mock realistic behavior + return Promise.resolve({ + status: 'SUCCESS', + message: 'Bazooka upgrade completed', + }); + }), }));
42-46: Enhance test suite setup with proper cleanup.Consider adding afterAll cleanup and importing types for better maintainability.
+import { upgradeBazooka } from '../src/helpers'; +import { logger } from '@dydxprotocol-indexer/base'; describe('Auxo Handler', () => { beforeEach(() => { jest.clearAllMocks(); }); + + afterAll(() => { + jest.restoreAllMocks(); + }); });
1-87: Consider architectural improvements for better testability.While the test implementation is functional, consider these architectural improvements:
- The tight coupling with AWS services makes testing more complex. Consider introducing an abstraction layer for AWS services.
- The test doesn't fully verify the integration with the newly refactored
helper.ts.Consider introducing a service interface:
interface CloudServiceProvider { invokeLambda(functionName: string, payload: any): Promise<any>; // Add other AWS service methods as needed } class AWSCloudService implements CloudServiceProvider { constructor(private lambdaClient: LambdaClient) {} async invokeLambda(functionName: string, payload: any): Promise<any> { // Implementation using AWS SDK } }This would make the handler more testable and the tests simpler:
class MockCloudService implements CloudServiceProvider { async invokeLambda(functionName: string, payload: any): Promise<any> { // Simple mock implementation } }indexer/services/auxo/src/helpers.ts (1)
95-96: Use existingsleepfunction for consistencyInstead of creating a new promise with
setTimeout, utilize the already definedsleepfunction for better code reuse and readability.Apply this diff:
- await new Promise((resolve) => setTimeout(resolve, 5000)); + await sleep(5000);
🛑 Comments failed to post (5)
indexer/services/auxo/__tests__/index.test.ts (1)
48-87:
⚠️ Potential issueImprove test coverage and error handling verification.
The current test case could be enhanced in several ways:
The mock context should include required properties
The error assertion should be more specific
Missing positive test cases
Enhance the context mock:
- const mockContext: Context = {} as any; // Mocked Context object + const mockContext: Context = { + awsRequestId: 'mock-request-id', + functionName: 'auxo-handler', + functionVersion: '1', + invokedFunctionArn: 'mock-arn', + memoryLimitInMB: '128', + callbackWaitsForEmptyEventLoop: true, + getRemainingTimeInMillis: () => 1000, + done: () => {}, + fail: () => {}, + succeed: () => {}, + };
- Make the error assertion more specific:
- await expect(handler(mockEvent, mockContext)).rejects.toThrow(); + await expect(handler(mockEvent, mockContext)).rejects.toThrow( + 'Bazooka Lambda execution failed: Some bazooka error' + );
- Add a positive test case:
it('should successfully handle Bazooka Lambda execution', async () => { mockClientSend.mockResolvedValueOnce({ StatusCode: 200, Payload: Buffer.from(JSON.stringify({ status: 'success' })), $metadata: { httpStatusCode: 200, requestId: 'mock-request-id-success', }, } as InvokeCommandOutput); const result = await handler(mockEvent, mockContext); expect(result).toEqual(expect.objectContaining({ statusCode: 200, body: expect.stringContaining('success'), })); });indexer/services/auxo/src/helpers.ts (4)
65-68:
⚠️ Potential issueHandle timeout in
upgradeBazookato ensure upgrade completionIf the
sleep(120000)promise resolves beforecheckLambdaStatus(lambda), theupgradeBazookafunction will complete without confirming whether the Lambda function was successfully updated. This may lead to an incomplete upgrade without any error notification.Apply this diff to handle the timeout scenario:
await Promise.race([ checkLambdaStatus(lambda), sleep(120000), ]); +throw new Error('Timed out waiting for Lambda function to update');This modification ensures that an error is thrown if the Lambda update does not complete within the expected time frame.
Committable suggestion was skipped due to low confidence.
259-260:
⚠️ Potential issueUse a robust method to construct the new image URI
The current approach
_.split(originalImage, ':')[0]assumes there's only one colon in the image URI, which might not be the case if the image includes a registry URL with a port or digest. This can result in incorrect image URIs.Consider parsing the image URI more reliably. Apply this diff:
- const newImage: string = `${_.split(originalImage, ':')[0]}:${event.upgrade_tag}`; + const imageParts = originalImage.split('@'); + const imageWithoutTag = imageParts[0].split(':')[0]; + const newImage: string = `${imageWithoutTag}:${event.upgrade_tag}`;This modification ensures that the new image URI is correctly constructed, even if the original image URI contains multiple colons or an image digest.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const imageParts = originalImage.split('@'); const imageWithoutTag = imageParts[0].split(':')[0]; const newImage: string = `${imageWithoutTag}:${event.upgrade_tag}`; updatedContainerDefinitions[serviceContainerDefinitionIndex].image = newImage;
143-143:
⚠️ Potential issueCorrect the parameter type for
lambdainrunDbAndKafkaMigrationThe
lambdaparameter is currently typed asECRClient, but it should beLambdaClientsince it invokes a Lambda function.Apply this diff to fix the parameter type:
export async function runDbAndKafkaMigration( createNewKafkaTopics: boolean, - lambda: ECRClient, + lambda: LambdaClient, ): Promise<void> {Committable suggestion was skipped due to low confidence.
334-369: 🛠️ Refactor suggestion
Utilize AWS SDK waiters instead of custom polling
The
waitForTaskDefinitionToRegisterfunction implements custom polling logic to check if the task definition is registered. AWS SDK provides built-in waiters that can simplify this process and handle retries more efficiently.Consider using the AWS SDK's waiter:
async function waitForTaskDefinitionToRegister( ecs: ECSClient, registerResult: RegisterTaskDefinitionCommandOutput, ): Promise<void> { - const taskDefinition = `${registerResult.taskDefinition!.family}:${registerResult.taskDefinition!.revision}`; - for (let i = 0; i <= config.MAX_TASK_DEFINITION_WAIT_TIME_MS; i += config.SLEEP_TIME_MS) { - const describeResult = await ecs.send( - new DescribeTaskDefinitionCommand({ - taskDefinition, - }), - ); - if (describeResult.taskDefinition !== undefined) { - logger.info({ - at: 'index#waitForTaskDefinitionToRegister', - message: 'Task definition registered', - taskDefinition, - describeResult, - }); - return; - } - logger.info({ - at: 'index#waitForTaskDefinitionToRegister', - message: `Task definition is undefined, sleeping ${config.SLEEP_TIME_MS}ms`, - }); - await sleep(config.SLEEP_TIME_MS); - } - logger.error({ - at: 'index#waitForTaskDefinitionToRegister', - message: 'Timed out waiting for task definition to register', - taskDefinition, - }); - throw new Error('Timed out waiting for task definition to register'); + await ecs.waitFor('tasksRegistered', { + taskDefinition: registerResult.taskDefinition!.taskDefinitionArn!, + $waiter: { + maxAttempts: config.MAX_TASK_DEFINITION_WAIT_TIME_MS / config.SLEEP_TIME_MS, + delay: config.SLEEP_TIME_MS / 1000, + }, + }); + logger.info({ + at: 'index#waitForTaskDefinitionToRegister', + message: 'Task definition registered', + }); }This refactor leverages the SDK's built-in functionality for waiting on resource states, improving code clarity and reliability.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
indexer/services/auxo/__tests__/index.test.ts (1)
22-34: Consider adding type safety to logger mocks.While the mocking implementation is functionally correct, consider adding TypeScript types to ensure type safety.
jest.mock('@dydxprotocol-indexer/base', () => { const actualModule = jest.requireActual('@dydxprotocol-indexer/base'); + type LoggerFn = (message: string, metadata?: object) => void; return { ...actualModule, logger: { - info: jest.fn(), - error: jest.fn(), - crit: jest.fn(), + info: jest.fn<void, Parameters<LoggerFn>>(), + error: jest.fn<void, Parameters<LoggerFn>>(), + crit: jest.fn<void, Parameters<LoggerFn>>(), }, startBugsnag: jest.fn(), }; });indexer/services/auxo/src/helpers.ts (4)
146-148: Update the 'at' field in logging to reflect the correct function nameThe
logger.infostatement uses'at': 'index#runDbMigration', but the function is now namedrunDbAndKafkaMigrationinhelpers.ts. Updating the'at'field improves log clarity and maintainability.Apply this diff to update the
'at'field:logger.info({ - at: 'index#runDbMigration', + at: 'helpers#runDbAndKafkaMigration', message: 'Running db migration', });
164-167: Update the 'at' field in error logging to reflect the correct function nameThe
logger.errorstatement references'at': 'index#runDbMigration'. Update it to match the current function name and file for accurate logging.Apply this diff to update the
'at'field:logger.error({ - at: 'index#runDbMigration', + at: 'helpers#runDbAndKafkaMigration', message: `Failed to run db migration due to bazooka failure: ${response.FunctionError}`, });
85-89: Update the 'at' field in logging to reflect the new file and function nameThe
logger.infostatement uses'at': 'index#upgradeBazooka', but since this function is now inhelpers.ts, updating the'at'value enhances clarity.Apply this diff to update the
'at'field:logger.info({ - at: 'index#upgradeBazooka', + at: 'helpers#checkLambdaStatus', message: 'Successfully upgraded bazooka', response: statusResponse, });
51-54: Update the 'at' field in logging to reflect the new file and function nameThe
logger.infostatement references'at': 'index#upgradeBazooka'. Updating this to reflect the current file and function improves maintainability.Apply this diff to update the
'at'field:logger.info({ - at: 'index#upgradeBazooka', + at: 'helpers#upgradeBazooka', message: `Upgrading bazooka to ${imageUri}`, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/services/auxo/tests/index.test.ts (1 hunks)
- indexer/services/auxo/src/helpers.ts (1 hunks)
- indexer/services/auxo/src/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
indexer/services/auxo/__tests__/index.test.ts (2)
9-20: LGTM: AWS SDK mocking setup is well-structured.The mock implementation correctly preserves the original module while allowing controlled testing of Lambda responses.
42-46: LGTM: Clean test setup.Proper test isolation is maintained by clearing mocks before each test.
indexer/services/auxo/src/index.ts (2)
1-3: AWS SDK Clients Imported CorrectlyThe imports for
ECRClient,ECSClient, andLambdaClientfrom the AWS SDK are appropriate and reflect the refactoring changes. Importing only the necessary clients improves code efficiency and maintainability.
8-10: Helper Functions Imported Properly from 'helpers.ts'The functions
upgradeBazooka,runDbAndKafkaMigration,createNewEcsTaskDefinitions, andupgradeEcsServicesare correctly imported from./helpers. This aligns with the refactoring objective to modularize support functions, enhancing code organization and testability.indexer/services/auxo/src/helpers.ts (1)
259-259: Verify the correctness of the new image URI constructionThe current method of constructing
newImagemay not handle images with complex tags or digests. Ensure that the new image URI correctly targets the desired image.Run the following script to check all image URIs:
✅ Verification successful
Let me analyze the code further by examining the context around this image URI construction.
This will help us understand:
- The context around how image URIs are processed
- If there are test cases validating this behavior
- The patterns of Docker image URIs used in the codebase
Let me search for more specific context around this image handling code.
This will help us understand:
- The complete context of the image URI transformation
- The event structure that provides the upgrade_tag
- Any test cases that validate this behavior
Based on the gathered context, I can now provide a final response:
The image URI construction is correct for the intended use case
The code is part of the Auxo service which handles ECS task definition updates. The image URI construction is appropriate because:
- The original image comes from an ECS container definition, which follows standard Docker image format
- The code only replaces the tag portion after the colon, preserving the repository path
- The
upgrade_tagis a validated field from theAuxoEventJsoninterface- The implementation is used in a controlled deployment process where the image format is standardized
The current implementation is sufficient for handling AWS ECR image URIs which follow the pattern:
[registry]/[repository]:[tag]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new image URIs are correctly formed. # Extract and display the image URIs from the task definitions. aws ecs describe-task-definitions --query 'taskDefinitionArns[*]' --output text | while read -r arn; do aws ecs describe-task-definition --task-definition "$arn" --query 'taskDefinition.containerDefinitions[*].image' --output text doneLength of output: 166
Script:
#!/bin/bash # Search for patterns of Docker image URI handling rg -A 5 -B 5 "originalImage.*split.*:" # Look for test cases related to image URI handling fd -e test.ts -e spec.ts -X rg -l "originalImage|newImage" # Search for any Docker image URI patterns in the codebase rg "docker.*:|registry.*:|gcr\.io|amazonaws\.com"Length of output: 9758
Script:
#!/bin/bash # Get more context around the image URI construction rg -B 10 -A 10 "newImage.*split.*originalImage" indexer/ # Look for the event type and structure ast-grep --pattern 'interface $_ { $$$ upgrade_tag: $_ $$$ }' # Find any related test cases specifically in auxo service fd -e test.ts -e spec.ts -p "auxo" -X catLength of output: 4967
| it('should return 500 when Bazooka Lambda errors', async () => { | ||
| mockClientSend.mockResolvedValueOnce({ | ||
| StatusCode: 500, | ||
| FunctionError: 'Some bazooka error', | ||
| $metadata: { | ||
| httpStatusCode: 200, // api returns 200 even if lambda runtime error | ||
| requestId: 'mock-request-id-invoke', | ||
| extendedRequestId: 'mock-extended-request-id-invoke', | ||
| cfId: 'mock-cf-id-invoke', | ||
| attempts: 1, | ||
| totalRetryDelay: 0, | ||
| }, | ||
| } as InvokeCommandOutput); | ||
|
|
||
| const mockEvent: APIGatewayEvent & AuxoEventJson = { | ||
| // APIGatewayEvent properties | ||
| body: null, | ||
| headers: {}, | ||
| multiValueHeaders: {}, | ||
| httpMethod: 'POST', | ||
| isBase64Encoded: false, | ||
| path: '/deploy', | ||
| pathParameters: null, | ||
| queryStringParameters: null, | ||
| multiValueQueryStringParameters: null, | ||
| stageVariables: null, | ||
| resource: '', | ||
| requestContext: {} as any, | ||
| // AuxoEventJson properties | ||
| upgrade_tag: 'some_tag', | ||
| prefix: 'some_prefix', | ||
| region: 'us-east-1', | ||
| regionAbbrev: 'us-east-1', | ||
| addNewKafkaTopics: false, | ||
| onlyRunDbMigrationAndCreateKafkaTopics: false, | ||
| }; | ||
|
|
||
| const mockContext: Context = {} as any; | ||
|
|
||
| await expect(handler(mockEvent, mockContext)).rejects.toThrow(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and clarity.
While the test covers the error path, consider these improvements:
- Make the test name more specific about the failure scenario
- Verify the specific error message or type
- Provide a more realistic mock context
- it('should return 500 when Bazooka Lambda errors', async () => {
+ it('should throw an error when Bazooka Lambda returns a runtime error with status 500', async () => {
mockClientSend.mockResolvedValueOnce({
StatusCode: 500,
FunctionError: 'Some bazooka error',
$metadata: {
httpStatusCode: 200,
requestId: 'mock-request-id-invoke',
extendedRequestId: 'mock-extended-request-id-invoke',
cfId: 'mock-cf-id-invoke',
attempts: 1,
totalRetryDelay: 0,
},
} as InvokeCommandOutput);
const mockEvent: APIGatewayEvent & AuxoEventJson = {
// ... event properties ...
};
- const mockContext: Context = {} as any;
+ const mockContext: Context = {
+ awsRequestId: 'mock-request-id',
+ functionName: 'auxo-handler',
+ functionVersion: '1',
+ invokedFunctionArn: 'arn:aws:lambda:us-east-1:mock',
+ memoryLimitInMB: '128',
+ logGroupName: '/aws/lambda/auxo-handler',
+ logStreamName: '2024/01/01/[$LATEST]mock',
+ callbackWaitsForEmptyEventLoop: true,
+ getRemainingTimeInMillis: () => 30000,
+ done: () => {},
+ fail: () => {},
+ succeed: () => {},
+ };
- await expect(handler(mockEvent, mockContext)).rejects.toThrow();
+ await expect(handler(mockEvent, mockContext)).rejects.toThrow(
+ 'Bazooka Lambda function failed with error: Some bazooka error'
+ );
});Also consider adding test cases for:
- Other error scenarios (e.g., network errors)
- Successful execution path
- Edge cases with different event properties
Committable suggestion was skipped due to low confidence.
| // handler.test.ts | ||
|
|
||
| import { handler } from '../src'; | ||
|
|
||
| import { InvokeCommandOutput } from '@aws-sdk/client-lambda'; | ||
| import { APIGatewayEvent, Context } from 'aws-lambda'; | ||
| import { AuxoEventJson } from 'src/types'; | ||
|
|
||
| // Mock aws clients from AWS SDK | ||
| const mockClientSend = jest.fn(); | ||
| jest.mock('@aws-sdk/client-lambda', () => { | ||
| const originalModule = jest.requireActual('@aws-sdk/client-lambda'); | ||
| return { | ||
| ...originalModule, | ||
| LambdaClient: jest.fn(() => ({ | ||
| send: mockClientSend, | ||
| })), | ||
| InvokeCommand: originalModule.InvokeCommand, | ||
| }; | ||
| }); | ||
|
|
||
| // Mock logger and startBugsnag from @dydxprotocol-indexer/base | ||
| jest.mock('@dydxprotocol-indexer/base', () => { | ||
| const actualModule = jest.requireActual('@dydxprotocol-indexer/base'); | ||
| return { | ||
| ...actualModule, // Spread the actual module's exports | ||
| logger: { | ||
| info: jest.fn(), | ||
| error: jest.fn(), | ||
| crit: jest.fn(), | ||
| }, | ||
| startBugsnag: jest.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| // Mock upgradeBazooka to do nothing | ||
| jest.mock('../src/helpers', () => ({ | ||
| ...jest.requireActual('../src/helpers'), | ||
| upgradeBazooka: jest.fn().mockResolvedValue(undefined), | ||
| })); | ||
|
|
||
| describe('Auxo Handler', () => { | ||
| beforeEach(() => { | ||
| // Reset all mocks before each test | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should return 500 when Bazooka Lambda errors', async () => { | ||
| mockClientSend.mockResolvedValueOnce({ | ||
| StatusCode: 500, | ||
| FunctionError: 'Some bazooka error', | ||
| $metadata: { | ||
| httpStatusCode: 200, // api returns 200 even if lambda runtime error | ||
| requestId: 'mock-request-id-invoke', | ||
| extendedRequestId: 'mock-extended-request-id-invoke', | ||
| cfId: 'mock-cf-id-invoke', | ||
| attempts: 1, | ||
| totalRetryDelay: 0, | ||
| }, | ||
| } as InvokeCommandOutput); | ||
|
|
||
| const mockEvent: APIGatewayEvent & AuxoEventJson = { | ||
| // APIGatewayEvent properties | ||
| body: null, | ||
| headers: {}, | ||
| multiValueHeaders: {}, | ||
| httpMethod: 'POST', | ||
| isBase64Encoded: false, | ||
| path: '/deploy', | ||
| pathParameters: null, | ||
| queryStringParameters: null, | ||
| multiValueQueryStringParameters: null, | ||
| stageVariables: null, | ||
| resource: '', | ||
| requestContext: {} as any, | ||
| // AuxoEventJson properties | ||
| upgrade_tag: 'some_tag', | ||
| prefix: 'some_prefix', | ||
| region: 'us-east-1', | ||
| regionAbbrev: 'us-east-1', | ||
| addNewKafkaTopics: false, | ||
| onlyRunDbMigrationAndCreateKafkaTopics: false, | ||
| }; | ||
|
|
||
| const mockContext: Context = {} as any; | ||
|
|
||
| await expect(handler(mockEvent, mockContext)).rejects.toThrow(); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now generate a final response.
Test coverage needed for runDbAndKafkaMigration and early return path
The implementation shows that runDbAndKafkaMigration has important logic for handling both database migrations and Kafka topic creation, but there's no test coverage for:
- The case where
createNewKafkaTopicsis true - The early return path when
onlyRunDbMigrationAndCreateKafkaTopicsis true
Suggested test cases to add:
- Test successful database migration with Kafka topic creation (
createNewKafkaTopics: true) - Test the early return path when
onlyRunDbMigrationAndCreateKafkaTopics: true - Test error handling when Kafka topic creation fails
🔗 Analysis chain
Verify test coverage for runDbAndKafkaMigration changes.
The PR mentions changes to runDbAndKafkaMigration, but the current test file focuses only on Bazooka failure. Additional test coverage might be needed.
Let me try a different search strategy to verify the test coverage.
Let me check the implementation and test coverage more specifically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing tests related to runDbAndKafkaMigration
echo "Searching for runDbAndKafkaMigration tests:"
rg "runDbAndKafkaMigration" --type ts --type test -C 3
echo -e "\nChecking implementation changes:"
ast-grep --pattern 'async function runDbAndKafkaMigration($_) {
$$$
}'
Length of output: 336
Script:
#!/bin/bash
# Search for runDbAndKafkaMigration in all TypeScript files
echo "Searching for runDbAndKafkaMigration in TypeScript files:"
rg "runDbAndKafkaMigration" --type ts -C 3
# Search for test files in the auxo service
echo -e "\nListing test files in auxo service:"
fd "\.test\.ts$" indexer/services/auxo
Length of output: 1737
Script:
#!/bin/bash
# Check the implementation of runDbAndKafkaMigration
echo "Checking runDbAndKafkaMigration implementation:"
cat indexer/services/auxo/src/helpers.ts
# Check test coverage for the case where onlyRunDbMigrationAndCreateKafkaTopics is true
echo -e "\nChecking test coverage for onlyRunDbMigrationAndCreateKafkaTopics:"
rg "onlyRunDbMigrationAndCreateKafkaTopics.*true" indexer/services/auxo/__tests__/index.test.ts -C 5
Length of output: 15416
| // Mock upgradeBazooka to do nothing | ||
| jest.mock('../src/helpers', () => ({ | ||
| ...jest.requireActual('../src/helpers'), | ||
| upgradeBazooka: jest.fn().mockResolvedValue(undefined), | ||
| })); |
There was a problem hiding this comment.
💡 Codebase verification
Now I have enough information to generate the final response. The helpers.ts file shows several exported functions that need test coverage, and the current test file only mocks upgradeBazooka.
Additional test coverage needed for helper functions
The current test only mocks upgradeBazooka, but there are other critical exported functions that should be tested:
runDbAndKafkaMigration: Handles database migrations and Kafka topic creationcreateNewEcsTaskDefinitions: Creates new ECS task definitionsupgradeEcsServices: Upgrades ECS services with new task definitions
These functions are crucial for the deployment process and should have proper test coverage to ensure reliable infrastructure updates.
🔗 Analysis chain
Verify test coverage for all helper functions.
Given that support functions were refactored into helper.ts, we should ensure adequate test coverage for all critical helper functions, not just upgradeBazooka.
Let me generate new shell scripts to find the correct helper file and its functions since the previous search found helpers from a different service (comlink).
Let me check the actual contents of the helpers.ts file to see all exported functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for exported functions in helper.ts that might need test coverage
# Find all exported functions in helper.ts
echo "Exported functions in helper.ts:"
ast-grep --pattern 'export async function $_($_) {
$$$
}'
# Check for existing test coverage
echo -e "\nExisting test coverage:"
rg "mock(Resolved|Rejected)Value.*helper" -A 2
Length of output: 5902
Script:
#!/bin/bash
# First, let's find the correct helper file
echo "Looking for helper file in auxo service:"
fd "helper" indexer/services/auxo/
# Then, let's examine its contents and exported functions
echo -e "\nExported functions in auxo helper file:"
ast-grep --pattern 'export $_ function $_($_) {
$$$
}' --lang typescript
# Check test coverage specifically in auxo tests
echo -e "\nTest coverage in auxo tests:"
rg "mock\(" indexer/services/auxo/__tests__/ -A 2
Length of output: 1275
Script:
#!/bin/bash
# Get the contents of the helpers file
echo "Contents of helpers.ts:"
cat indexer/services/auxo/src/helpers.ts
# Also check for any other test files that might be testing helper functions
echo -e "\nOther test files testing helpers:"
rg "helpers" indexer/services/auxo/__tests__/ -l
Length of output: 15294
| logger.info({ | ||
| at: 'index#createNewEcsTaskDefinition', | ||
| message: 'Got existing ECS Task Definition', | ||
| taskDefinitionName, | ||
| describeResult, | ||
| }); |
There was a problem hiding this comment.
Avoid logging extensive task definition details
Logging the entire describeResult can be excessive. To enhance performance and security, log only necessary information.
Apply this diff to refine the logged data:
logger.info({
at: 'index#createNewEcsTaskDefinition',
message: 'Got existing ECS Task Definition',
taskDefinitionName,
- describeResult,
+ taskDefinitionArn: describeResult.taskDefinition?.taskDefinitionArn,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info({ | |
| at: 'index#createNewEcsTaskDefinition', | |
| message: 'Got existing ECS Task Definition', | |
| taskDefinitionName, | |
| describeResult, | |
| }); | |
| logger.info({ | |
| at: 'index#createNewEcsTaskDefinition', | |
| message: 'Got existing ECS Task Definition', | |
| taskDefinitionName, | |
| taskDefinitionArn: describeResult.taskDefinition?.taskDefinitionArn, | |
| }); |
| logger.info({ | ||
| at: 'index#getImageDetail', | ||
| message: 'Successfully got ecr images', | ||
| images, | ||
| repositoryName, | ||
| event, | ||
| }); |
There was a problem hiding this comment.
Limit logging of large data structures to prevent performance issues
Logging the entire images object can impact performance and may expose sensitive information. Consider logging only essential details.
Apply this diff to log relevant information:
logger.info({
at: 'index#getImageDetail',
message: 'Successfully got ecr images',
- images,
+ imageDigests: images.imageDetails?.map(detail => detail.imageDigest),
repositoryName,
event,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info({ | |
| at: 'index#getImageDetail', | |
| message: 'Successfully got ecr images', | |
| images, | |
| repositoryName, | |
| event, | |
| }); | |
| logger.info({ | |
| at: 'index#getImageDetail', | |
| message: 'Successfully got ecr images', | |
| imageDigests: images.imageDetails?.map(detail => detail.imageDigest), | |
| repositoryName, | |
| event, | |
| }); |
| } | ||
|
|
||
| // Wait for 5s before checking again | ||
| await new Promise((resolve) => setTimeout(resolve, 5000)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the 'sleep' helper function instead of duplicating sleep logic
In checkLambdaStatus, you're manually implementing a sleep with setTimeout. Since a sleep helper function is already defined, consider using it for consistency and code reuse.
Apply this diff to use the sleep function:
- await new Promise((resolve) => setTimeout(resolve, 5000));
+ await sleep(5000);Committable suggestion was skipped due to low confidence.
| const updatedContainerDefinitions: ContainerDefinition[] = _.cloneDeep( | ||
| taskDefinition.containerDefinitions!, | ||
| ); | ||
| const newImage: string = `${_.split(originalImage, ':')[0]}:${event.upgrade_tag}`; | ||
| updatedContainerDefinitions[serviceContainerDefinitionIndex].image = newImage; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure image tag parsing is robust
Splitting the image string using _.split(originalImage, ':')[0] may not handle all image URI formats, such as those with port numbers or digest references. Consider using a more reliable method to construct the new image URI.
Apply this diff to improve image URI handling:
const originalImage: string = serviceContainerDefinition.image;
-const newImage: string = `${_.split(originalImage, ':')[0]}:${event.upgrade_tag}`;
+const imageParts = originalImage.split('/');
+const imageNameWithTag = imageParts.pop()?.split(':')[0];
+const newImage = `${imageParts.join('/')}/${imageNameWithTag}:${event.upgrade_tag}`;
updatedContainerDefinitions[serviceContainerDefinitionIndex].image = newImage;Committable suggestion was skipped due to low confidence.
jerryfan01234
force-pushed
the
auxo-bazooka-error
branch
from
45b90f8 to
893aa1a
Compare
jerryfan01234
force-pushed
the
auxo-bazooka-error
branch
from
893aa1a to
004ffc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
indexer/services/auxo/__tests__/index.test.ts (1)
22-26: Enhance test suite structure.Consider adding:
afterEach/afterAllcleanup to restore mocks- Nested
describeblocks for different scenarios (e.g., "error cases", "success cases")describe('Auxo Handler', () => { beforeEach(() => { jest.clearAllMocks(); }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe('error cases', () => { + // Move error test cases here + }); + + describe('success cases', () => { + // Add success test cases here + }); });indexer/services/auxo/src/index.ts (2)
Line range hint
17-17: Remove unnecessary ESLint directive.The
// eslint-disable-next-line @typescript-eslint/require-awaitcomment is unnecessary since thehandlerfunction containsawaitexpressions.Apply this diff to remove the unnecessary ESLint directive:
-// eslint-disable-next-line @typescript-eslint/require-await
Line range hint
28-30: Enhance error logging for better traceability.Consider including additional context from the
eventobject in the error log to facilitate debugging.Modify the error logging as follows:
logger.error({ at: 'index#handler', message: 'Error upgrading services', + event, error, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/services/auxo/tests/index.test.ts (1 hunks)
- indexer/services/auxo/src/helpers.ts (1 hunks)
- indexer/services/auxo/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/services/auxo/src/helpers.ts
🧰 Additional context used
🔇 Additional comments (3)
indexer/services/auxo/src/index.ts (3)
1-3: AWS SDK Client imports are correctly added.The imports for
ECRClient,ECSClient, andLambdaClientfrom the AWS SDK are appropriately added to interact with AWS services.
10-10: Confirm 'AuxoEventJson' includes all required properties.Ensure that the
AuxoEventJsoninterface includesregion,addNewKafkaTopics, andonlyRunDbMigrationAndCreateKafkaTopicsas they are used in thehandlerfunction.Run the following script to verify that
AuxoEventJsonincludes the necessary properties:✅ Verification successful
Let me try a different approach to find the interface definition.
AuxoEventJsoninterface has all the required propertiesThe interface definition in
indexer/services/auxo/src/types.tsincludes all the properties mentioned in the review comment:
region: stringaddNewKafkaTopics: booleanonlyRunDbMigrationAndCreateKafkaTopics: booleanAdditionally, it includes other properties:
upgrade_tag: stringprefix: stringregionAbbrev: string🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'AuxoEventJson' interface includes specific properties. # Test: Search for 'AuxoEventJson' interface definition with required properties. ast-grep --pattern $'interface AuxoEventJson { region: $_; addNewKafkaTopics: $_; onlyRunDbMigrationAndCreateKafkaTopics: $_; $$$ }' --lang tsLength of output: 151
Script:
#!/bin/bash # Search for AuxoEventJson interface definition rg "interface AuxoEventJson" -A 10 # Also search for type definition as an alternative rg "type AuxoEventJson" -A 10Length of output: 768
8-9: Ensure all helper functions are properly exported from 'helpers.ts'.Verify that
helpers.tsexportsupgradeBazooka,runDbAndKafkaMigration,createNewEcsTaskDefinitions, andupgradeEcsServicesto prevent import errors.Run the following script to confirm that all required functions are exported from
helpers.ts:✅ Verification successful
Let me try a different approach to verify these exports.
This will help us:
- Locate the helpers.ts file
- Search for export statements of these specific functions with some context
All required helper functions are properly exported
The verification results show that all four functions (
upgradeBazooka,runDbAndKafkaMigration,createNewEcsTaskDefinitions, andupgradeEcsServices) are correctly exported as async functions inindexer/services/auxo/src/helpers.ts. The imports inindex.tswill work as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'helpers.ts' exports the necessary functions. # Test: Search for export statements in 'helpers.ts' that include the required functions. ast-grep --pattern $'export async function upgradeBazooka($$$) { $$$ } export async function runDbAndKafkaMigration($$$) { $$$ } export async function createNewEcsTaskDefinitions($$$) { $$$ } export async function upgradeEcsServices($$$) { $$$ }' --lang tsLength of output: 920
Script:
#!/bin/bash # First, let's find the helpers.ts file fd helpers.ts # Then, let's examine its content focusing on export statements rg "export.*function (upgradeBazooka|runDbAndKafkaMigration|createNewEcsTaskDefinitions|upgradeEcsServices)" -A 1Length of output: 2802
| it('should return 500 when Bazooka Lambda errors', async () => { | ||
| // Mock upgradeBazooka to do nothing | ||
| jest.spyOn(helpers, 'upgradeBazooka').mockResolvedValue(undefined); | ||
|
|
||
| // Mock LambdaClient.send to return response with FunctionError | ||
| jest.spyOn(LambdaClient.prototype, 'send').mockImplementation(() => { | ||
| return { | ||
| StatusCode: 500, | ||
| FunctionError: 'Some bazooka error', | ||
| $metadata: { | ||
| httpStatusCode: 200, // api returns 200 even if lambda runtime error | ||
| requestId: 'mock-request-id-invoke', | ||
| extendedRequestId: 'mock-extended-request-id-invoke', | ||
| cfId: 'mock-cf-id-invoke', | ||
| attempts: 1, | ||
| totalRetryDelay: 0, | ||
| }, | ||
| } as InvokeCommandOutput; | ||
| }); | ||
|
|
||
| const mockEvent: APIGatewayEvent & AuxoEventJson = { | ||
| // APIGatewayEvent properties | ||
| body: null, | ||
| headers: {}, | ||
| multiValueHeaders: {}, | ||
| httpMethod: 'POST', | ||
| isBase64Encoded: false, | ||
| path: '/deploy', | ||
| pathParameters: null, | ||
| queryStringParameters: null, | ||
| multiValueQueryStringParameters: null, | ||
| stageVariables: null, | ||
| resource: '', | ||
| requestContext: {} as any, | ||
| // AuxoEventJson properties | ||
| upgrade_tag: 'some_tag', | ||
| prefix: 'some_prefix', | ||
| region: 'us-east-1', | ||
| regionAbbrev: 'us-east-1', | ||
| addNewKafkaTopics: false, | ||
| onlyRunDbMigrationAndCreateKafkaTopics: false, | ||
| }; | ||
|
|
||
| const mockContext: Context = {} as any; | ||
|
|
||
| await expect(handler(mockEvent, mockContext)).rejects.toThrow('bazooka failure: Some bazooka error'); |
There was a problem hiding this comment.
Add comprehensive test coverage.
The current test file only covers the Bazooka Lambda error case. Based on the PR objectives and changes, we need additional test coverage for:
-
Success scenarios:
- Successful deployment without Kafka topics
- Successful deployment with new Kafka topics
- Successful database migration only
-
Error scenarios:
- Database migration failures
- ECS service upgrade failures
- Network/timeout errors
-
Edge cases:
- Invalid event properties
- Missing required fields
- Unexpected Lambda responses
Here's a template for additional test cases:
// Success scenarios
it('should successfully complete deployment without Kafka topics', async () => {
// Mock successful responses
jest.spyOn(helpers, 'upgradeBazooka').mockResolvedValue(undefined);
jest.spyOn(helpers, 'runDbAndKafkaMigration').mockResolvedValue(undefined);
const mockEvent = {
...baseMockEvent,
addNewKafkaTopics: false,
};
await expect(handler(mockEvent, mockContext)).resolves.not.toThrow();
expect(helpers.upgradeBazooka).toHaveBeenCalledTimes(1);
expect(helpers.runDbAndKafkaMigration).toHaveBeenCalledWith(
expect.objectContaining({ createNewKafkaTopics: false })
);
});
it('should only run database migration when onlyRunDbMigrationAndCreateKafkaTopics is true', async () => {
// ... similar structure for other cases
});| import * as helpers from '../src/helpers'; | ||
| import { InvokeCommandOutput, LambdaClient } from '@aws-sdk/client-lambda'; | ||
| import { APIGatewayEvent, Context } from 'aws-lambda'; | ||
| import { AuxoEventJson } from 'src/types'; |
There was a problem hiding this comment.
Fix the import path for types.
The import path 'src/types' appears to be absolute. It should be relative to maintain consistency and avoid potential module resolution issues.
-import { AuxoEventJson } from 'src/types';
+import { AuxoEventJson } from '../src/types';Committable suggestion was skipped due to low confidence.
| onlyRunDbMigrationAndCreateKafkaTopics: false, | ||
| }; | ||
|
|
||
| const mockContext: Context = {} as any; |
There was a problem hiding this comment.
Improve mock context implementation.
The current mock context is too minimal ({} as any), which could hide potential issues. Implement a proper mock context with all required properties.
-const mockContext: Context = {} as any;
+const mockContext: Context = {
+ awsRequestId: 'mock-request-id',
+ functionName: 'auxo-handler',
+ functionVersion: '1',
+ invokedFunctionArn: 'arn:aws:lambda:us-east-1:123456789012:function:auxo-handler',
+ memoryLimitInMB: '128',
+ logGroupName: '/aws/lambda/auxo-handler',
+ logStreamName: '2024/01/01/[$LATEST]mock',
+ callbackWaitsForEmptyEventLoop: true,
+ getRemainingTimeInMillis: () => 30000,
+ done: jest.fn(),
+ fail: jest.fn(),
+ succeed: jest.fn(),
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockContext: Context = {} as any; | |
| const mockContext: Context = { | |
| awsRequestId: 'mock-request-id', | |
| functionName: 'auxo-handler', | |
| functionVersion: '1', | |
| invokedFunctionArn: 'arn:aws:lambda:us-east-1:123456789012:function:auxo-handler', | |
| memoryLimitInMB: '128', | |
| logGroupName: '/aws/lambda/auxo-handler', | |
| logStreamName: '2024/01/01/[$LATEST]mock', | |
| callbackWaitsForEmptyEventLoop: true, | |
| getRemainingTimeInMillis: () => 30000, | |
| done: jest.fn(), | |
| fail: jest.fn(), | |
| succeed: jest.fn(), | |
| }; |
Changelist
refactored support functions to helper.ts for mocking
added unit test
THE ONLY CHANGE to the functions in helper.ts is
runDbAndKafkaMigrationwhere function runtime exceptions now throw errorTest Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Refactor