Add function to fetch availability zone id (#2326) #2390
Conversation
WalkthroughThe changes introduce a new dependency on the AWS SDK for EC2, add a function to retrieve availability zone IDs for AWS ECS containers, and expand the configuration schema to include the AWS region. Additionally, the module exports have been updated to include the new functionality from the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ECS
participant EC2
App->>ECS: Check environment variables
ECS-->>App: Return metadata URI and region
App->>ECS: Construct metadata URL
App->>ECS: Fetch availability zone name
ECS-->>App: Return zone name
App->>EC2: Describe availability zones
EC2-->>App: Return zone ID
App-->>ECS: Log and return zone ID
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
indexer/packages/base/src/config.ts (1)
42-42: LGTM! Consider adding documentation and evaluating requirements.The addition of the
AWS_REGIONproperty is consistent with the existing schema structure and uses appropriate parsing.Consider the following improvements:
- Add a comment explaining the purpose and usage of this property.
- Evaluate if this property should be required in certain environments (e.g., PRODUCTION, STAGING).
- Consider adding validation for the AWS region format if necessary.
Example implementation with improvements:
// AWS region for services requiring AWS SDK integration AWS_REGION: parseString({ default: '', requireInEnv: [NodeEnv.PRODUCTION, NodeEnv.STAGING], validate: (value) => /^[a-z]{2}-[a-z]+-\d$/.test(value) || value === '', }),indexer/packages/base/src/az-id.ts (2)
8-8: Use truthiness checks for environment variablesInstead of comparing environment variables to empty strings, consider using truthiness checks to handle cases where the variables might be
undefinedornull.Modify the condition as follows:
- if (config.ECS_CONTAINER_METADATA_URI_V4 !== '' && config.AWS_REGION !== '') { + if (config.ECS_CONTAINER_METADATA_URI_V4 && config.AWS_REGION) {
7-51: Consider adding unit tests for the new functionThe
getAvailabilityZoneIdfunction interacts with AWS services and handles multiple error scenarios. To ensure its reliability and correct behavior, especially when dependencies fail or environment variables are missing, consider adding unit tests that cover:
- Successful retrieval of the availability zone ID.
- Handling of missing or empty environment variables.
- Responses when AWS services return errors.
- Behavior when
AvailabilityZonesis undefined or empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
- indexer/packages/base/package.json (1 hunks)
- indexer/packages/base/src/az-id.ts (1 hunks)
- indexer/packages/base/src/config.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
🔇 Additional comments (2)
indexer/packages/base/src/index.ts (1)
17-17: LGTM! Verify theaz-idmodule.The new export statement is correctly added and aligns with the PR objective of adding a function to fetch the availability zone ID.
Let's verify the existence and content of the
az-idmodule:This script will help us confirm that the
az-idmodule exists and contains the expected functionality for fetching the availability zone ID.indexer/packages/base/package.json (1)
35-35: Consider the implications of adding the AWS SDK dependency.The addition of
@aws-sdk/client-ec2aligns with the PR objective of fetching the availability zone ID. However, please consider the following points:
- Impact on project size and build time: The AWS SDK is a large dependency. Ensure that this doesn't significantly increase your build times or bundle size.
- Security: As with any new dependency, be aware of potential security implications. Regularly update this package and monitor for any security advisories.
- Scope: Consider if this dependency is necessary for the entire project. If it's only used in a specific module, you might want to move it to a more localized
package.jsonto avoid unnecessary imports in other parts of the project.To verify the necessity and usage of this new dependency, you can run the following script:
This will help ensure that the dependency is actually being used in the codebase and identify where it's being imported.
✅ Verification successful
Adjusting the search commands to target specific file extensions.
The previous scripts encountered errors with the
--typeflag. Let's try searching by file extensions instead:
Dependency
@aws-sdk/client-ec2Usage ConfirmedThe
@aws-sdk/client-ec2package is actively used insrc/az-id.tsfor fetching the availability zone ID, aligning with the PR objectives. The usage is appropriately localized within thebasepackage, ensuring no unnecessary imports elsewhere in the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of @aws-sdk/client-ec2 in the codebase # Test: Search for imports of the EC2 client echo "Searching for EC2 client imports:" rg --type typescript --type javascript "from\s+['\"]@aws-sdk/client-ec2['\"]" -g '!node_modules' # Test: Search for any other usage of the EC2 namespace echo "Searching for other EC2 namespace usage:" rg --type typescript --type javascript "EC2" -g '!node_modules' -g '!*.json'Length of output: 417
Script:
# Searching for EC2 client imports in .ts, .tsx, .js, .jsx files rg "from\s+['\"]@aws-sdk/client-ec2['\"]" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -g "!node_modules" # Searching for EC2 namespace usage in .ts, .tsx, .js, .jsx files rg "EC2" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -g "!node_modules" -g "!*.json"Length of output: 633
| } catch (error) { | ||
| logger.error({ | ||
| at: 'az-id#getAvailabilityZoneId', | ||
| message: 'Failed to fetch availabilty zone id from EC2. ', |
There was a problem hiding this comment.
Typographical error in log message
There's a typo in the log message: "availabilty" should be "availability".
Apply the following diff to fix the typo:
- message: 'Failed to fetch availabilty zone id from EC2. ',
+ message: 'Failed to fetch availability zone id from EC2. ',📝 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.
| message: 'Failed to fetch availabilty zone id from EC2. ', | |
| message: 'Failed to fetch availability zone id from EC2. ', |
| } catch (error) { | ||
| logger.error({ | ||
| at: 'az-id#getAvailabilityZoneId', | ||
| message: 'Failed to retrieve availability zone from metadata endpoint. No availabilty zone id found.', |
There was a problem hiding this comment.
Typographical error in log message
There's a typo in the log message: "availabilty" should be "availability".
Apply the following diff to fix the typo:
- message: 'Failed to retrieve availability zone from metadata endpoint. No availabilty zone id found.',
+ message: 'Failed to retrieve availability zone from metadata endpoint. No availability zone id found.',📝 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.
| message: 'Failed to retrieve availability zone from metadata endpoint. No availabilty zone id found.', | |
| message: 'Failed to retrieve availability zone from metadata endpoint. No availability zone id found.', |
| } else { | ||
| logger.error({ | ||
| at: 'az-id#getAvailabilityZoneId', | ||
| message: 'No metadata URI or region. No availabilty zone id found.', |
There was a problem hiding this comment.
Typographical error in log message
There's a typo in the log message: "availabilty" should be "availability".
Apply the following diff to fix the typo:
- message: 'No metadata URI or region. No availabilty zone id found.',
+ message: 'No metadata URI or region. No availability zone id found.',📝 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.
| message: 'No metadata URI or region. No availabilty zone id found.', | |
| message: 'No metadata URI or region. No availability zone id found.', |
| const zoneId = ec2Response.AvailabilityZones![0].ZoneId!; | ||
| logger.info({ | ||
| at: 'az-id#getAvailabilityZoneId', | ||
| message: `Got availability zone id ${zoneId}.`, | ||
| }); | ||
| return ec2Response.AvailabilityZones![0].ZoneId!; |
There was a problem hiding this comment.
Ensure AvailabilityZones data is valid before accessing
The code assumes that ec2Response.AvailabilityZones is defined and contains at least one element. Accessing ec2Response.AvailabilityZones![0].ZoneId! without proper checks may lead to runtime errors if the array is undefined or empty. Please add a check to confirm that ec2Response.AvailabilityZones exists and has at least one element before accessing it.
Consider modifying the code as follows:
- const zoneId = ec2Response.AvailabilityZones![0].ZoneId!;
- logger.info({
- at: 'az-id#getAvailabilityZoneId',
- message: `Got availability zone id ${zoneId}.`,
- });
- return ec2Response.AvailabilityZones![0].ZoneId!;
+ if (
+ ec2Response.AvailabilityZones &&
+ ec2Response.AvailabilityZones.length > 0 &&
+ ec2Response.AvailabilityZones[0].ZoneId
+ ) {
+ const zoneId = ec2Response.AvailabilityZones[0].ZoneId;
+ logger.info({
+ at: 'az-id#getAvailabilityZoneId',
+ message: `Got availability zone id ${zoneId}.`,
+ });
+ return zoneId;
+ } else {
+ logger.error({
+ at: 'az-id#getAvailabilityZoneId',
+ message: 'AvailabilityZones is undefined or empty.',
+ });
+ return '';
+ }📝 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 zoneId = ec2Response.AvailabilityZones![0].ZoneId!; | |
| logger.info({ | |
| at: 'az-id#getAvailabilityZoneId', | |
| message: `Got availability zone id ${zoneId}.`, | |
| }); | |
| return ec2Response.AvailabilityZones![0].ZoneId!; | |
| if ( | |
| ec2Response.AvailabilityZones && | |
| ec2Response.AvailabilityZones.length > 0 && | |
| ec2Response.AvailabilityZones[0].ZoneId | |
| ) { | |
| const zoneId = ec2Response.AvailabilityZones[0].ZoneId; | |
| logger.info({ | |
| at: 'az-id#getAvailabilityZoneId', | |
| message: `Got availability zone id ${zoneId}.`, | |
| }); | |
| return zoneId; | |
| } else { | |
| logger.error({ | |
| at: 'az-id#getAvailabilityZoneId', | |
| message: 'AvailabilityZones is undefined or empty.', | |
| }); | |
| return ''; | |
| } |
|
https://github.com/Mergifyio release/indexer/v5.x |
|
https://github.com/Mergifyio release/indexer/v7.x |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
|
https://github.com/Mergifyio backport release/indexer/v5.x |
🛑 Command
|
|
https://github.com/Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
|
https://github.com/Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
…2410) Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com>
…2409) Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
Changelist
[Describe or list the changes made in this PR]
Test 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
az-idmodule.