Skip to content

Conversation

@MtMath
Copy link
Contributor

@MtMath MtMath commented Apr 3, 2025

Pull Request: Migrate AWS SDK from v2 to v3

I've avoided affecting existing code and functionality as much as possible by keeping the changes to aws-sdk v3 only.

Changes

  • Replaced aws-sdk dependency with modular SDK v3 packages
  • Updated import statements to use specific service clients and commands
  • Converted method calls to command pattern (client.send(new Command()))
  • Updated waiters to use new waitUntil functions
  • Added null checks for potentially undefined properties

Files Changed

  • package.json: Updated dependencies
- aws-sdk
+ @aws-sdk/client-cloudformation
+ @aws-sdk/client-cloudwatch-logs
+ @aws-sdk/client-kinesis
+ @aws-sdk/client-ecs
+ @aws-sdk/client-s3
  • src/model/cloud-runner/providers/aws/**/*.ts: Updated AWS SDK imports and usage patterns
  • Added some checks, but didn't set them to stop the tasks. CreationTime field.
if (!element.CreationTime) {
  CloudRunnerLogger.log(`${element.StackName} due to undefined CreationTime`);
}

Testing


Considerations

  • Consider adopting Pulumi for infrastructure-as-code to simplify AWS management and improve maintainability.
  • Currently, it is very difficult to test the functionalities of AWS, as it requires a lot of configuration. ( at least for me, but that may be because it's my first time ).

Summary by CodeRabbit

  • Chores
    • Upgraded AWS service integrations with the latest SDK modules.
  • Refactor
    • Enhanced backend cloud operations for improved performance, reliability, and error handling in managing tasks and resources.
    • Transitioned to a more modular and command-based approach for AWS SDK usage.

These updates help ensure more efficient and stable cloud interactions, contributing to a smoother overall experience.

@coderabbitai
Copy link

coderabbitai bot commented Apr 3, 2025

📝 Walkthrough

Walkthrough

This pull request migrates the project from the legacy AWS SDK to the modular AWS SDK v3. New dependencies were added in the package configuration, and multiple modules have been refactored to use command-based API calls (via the .send() method) instead of promise-based calls. Key changes include updated type definitions, modified method signatures, and revised waiting mechanisms for stack operations and task management across CloudFormation, ECS, and Kinesis integrations.

Changes

File(s) Change Summary
package.json Added new AWS SDK v3 dependencies: @aws-sdk/client-cloudformation, @aws-sdk/client-cloudwatch-logs, @aws-sdk/client-ecs, @aws-sdk/client-kinesis, and @aws-sdk/client-s3.
src/.../aws-base-stack.ts,
src/.../aws-error.ts,
src/.../aws-job-stack.ts,
src/.../index.ts,
src/.../services/garbage-collection-service.ts,
src/.../services/task-service.ts
Replaced legacy AWS SDK usage with modular command-based API calls. Updated method signatures and types to use new command objects (e.g., ListStacksCommand, CreateStackCommand) and wait methods (waitUntilStackCreateComplete, waitUntilStackDeleteComplete).
src/.../aws-task-runner.ts Updated ECS and Kinesis integrations: modified static property types and method parameters; replaced .promise() calls with .send(new Command(...)) to handle task execution and log handling.
src/.../cloud-runner-aws-task-def.ts Changed type definitions for taskDefResources and baseResources from `AWS.CloudFormation.StackResources

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant CF as CloudFormation Client
    participant CFService as AWS CloudFormation Service

    Client->>CF: send(ListStacksCommand)
    CF->>CFService: Request current stacks
    CFService-->>CF: Return stack list
    alt Stack creation required
        Client->>CF: send(CreateStackCommand)
        CF->>CFService: Create new stack
        CFService-->>CF: Return creation response
        Client->>CF: waitUntilStackCreateComplete({client, maxWaitTime})
    end
Loading
sequenceDiagram
    participant Runner as TaskRunner
    participant ECS as ECS Client
    participant ECSService as AWS ECS Service
    participant Kinesis as Kinesis Client
    participant KinesisService as AWS Kinesis Service

    Runner->>ECS: send(RunTaskCommand)
    ECS->>ECSService: Start task execution
    ECSService-->>ECS: Return task details
    Runner->>ECS: send(DescribeTasksCommand)
    ECSService-->>ECS: Return task status
    Runner->>Kinesis: send(GetRecordsCommand)
    Kinesis->>KinesisService: Fetch log records
    KinesisService-->>Kinesis: Return log records
Loading

Poem

I'm a bouncy rabbit, coding away,
With new AWS commands, I brighten my day.
No more promises left to hang or wait,
Modular commands now keep my code in shape!
Hopping through stacks and tasks with glee,
Carrots and clean code—what a joy to be!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db96d97 and bb2bf25.

📒 Files selected for processing (1)
  • src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link

github-actions bot commented Apr 3, 2025

Cat Gif

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (2)
package.json (1)

35-43: 💡 Verification agent

❓ Verification inconclusive

Consider removing the deprecated aws-sdk v2 package.

While you've successfully added the AWS SDK v3 modules, the old aws-sdk package (v2) is still present at line 43. After completing the migration, you should consider removing this dependency to avoid confusion and reduce your package size.


🏁 Script executed:

#!/bin/bash
# Verify if aws-sdk v2 is still being imported anywhere after migration
rg -l "require\('aws-sdk'\)" --type ts
rg -l "from 'aws-sdk'" --type ts

Length of output: 87


Below is a new script to widen the search (across all file types, not just TypeScript) for any residual usage of the deprecated aws-sdk v2:

Once you run the above script, please verify that it produces no output. If no files are listed, it confirms that there are no remaining import statements referencing aws-sdk v2.


Final Review Comment:

Remove unused aws-sdk v2 dependency

While migrating to AWS SDK v3, the old aws-sdk v2 package (listed at line 43 in package.json) appears to be completely unused in the codebase. Our verification (including the extended search across all file types) did not reveal any references to it. To avoid confusion and reduce your package size, please remove the aws-sdk dependency.


🏁 Script executed:

#!/bin/bash
# Search for CommonJS and ES Module import patterns for aws-sdk, excluding package.json
rg -l -g '!package.json' "require\(['\"]aws-sdk['\"]\)" .
rg -l -g '!package.json' "from ['\"]aws-sdk['\"]" .

Length of output: 243


Below is a revised review comment that incorporates the extended verification:


Remove Deprecated aws-sdk v2 Dependency

After verifying the codebase using extended searches (both CommonJS and ES module import patterns across all file types excluding package.json), no references to the deprecated aws-sdk v2 were found. This confirms that the migration to AWS SDK v3 is complete and the legacy dependency is unused. Please remove "aws-sdk": "^2.1081.0" from your package.json (line 43) to avoid confusion and reduce package size.

  • Action Item: Remove the aws-sdk dependency from package.json at line 43.
src/model/cloud-runner/providers/aws/services/task-service.ts (1)

59-67: ⚠️ Potential issue

Fix inaccurate “age” calculation.

Using new Date(Date.now() - creationTime) and then calling getHours()/getMinutes() returns the local clock time, not total age. Replace it with direct time-difference calculations to accurately track days/hours/minutes since creation.

Apply this diff as a sample fix:

-      const ageDate: Date = new Date(Date.now() - (element.CreationTime?.getTime() ?? 0));
-      CloudRunnerLogger.log(
-        `Task Stack ${element.StackName} - Age D${Math.floor(
-          ageDate.getHours() / 24,
-        )} H${ageDate.getHours()} M${ageDate.getMinutes()}`
-      );
+      const differenceMs = Date.now() - (element.CreationTime?.getTime() ?? 0);
+      const diffHours = Math.floor(differenceMs / (1000 * 60 * 60));
+      const diffMinutes = Math.floor(differenceMs / (1000 * 60)) % 60;
+      const diffDays = Math.floor(diffHours / 24);
+      CloudRunnerLogger.log(
+        `Task Stack ${element.StackName} - Age D${diffDays} H${diffHours % 24} M${diffMinutes}`
+      );
🧹 Nitpick comments (20)
src/model/cloud-runner/providers/aws/services/garbage-collection-service.ts (1)

71-71: Consider error handling for failed delete operations.

While the migration to AWS SDK v3 is correct, you might want to add error handling for the delete operations, as they could fail for various reasons (e.g., insufficient permissions, resource dependencies).

-        await cwl.send(new DeleteLogGroupCommand({ logGroupName: element.logGroupName || '' }));
+        try {
+          await cwl.send(new DeleteLogGroupCommand({ logGroupName: element.logGroupName || '' }));
+        } catch (error) {
+          CloudRunnerLogger.error(`Failed to delete log group ${element.logGroupName}: ${error.message}`);
+        }
src/model/cloud-runner/providers/aws/index.ts (5)

80-80: Consider re-using the CloudFormation client for performance.

Re-initializing a new CloudFormation client can be expensive if called frequently. Unless isolation across invocations is needed, re-using a shared client instance might improve efficiency.


94-95: Multiple AWS clients in the same scope.

While this approach is valid, you might unify the ECS client and CF client creation in a single place for consistency and maintainability, preventing accidental region mismatches.


97-97: Assigning Kinesis client statically to AwsTaskRunner.

Attaching the Kinesis client to AwsTaskRunner is okay, but consider dependency injection if you want to keep the runner loosely coupled and easier to test.


138-138: Deleting cleanup stack separately.

This is consistent with the new approach, but ensure you have logic ensuring the cleanup stack itself is fully inactive before removal to avoid “stack in use” errors.


150-158: Waiting for cleanup stack deletion.

Same approach for the cleanup stack. Check whether 200 seconds is long enough, especially in high-load scenarios or large resource dependencies.

src/model/cloud-runner/providers/aws/aws-base-stack.ts (2)

62-62: Describing stacks with DescribeStacksCommand.

This asynchronous call is straightforward. Make sure thorough error handling is in place if the stack is absent.


61-61: Use lowercase boolean instead of Boolean.

Static analysis flags Boolean as a banned type. For consistency and clarity, prefer the primitive boolean.

- const stackExists: Boolean = stackNames.includes(baseStackName) || false;
+ const stackExists: boolean = stackNames.includes(baseStackName) || false;
src/model/cloud-runner/providers/aws/aws-job-stack.ts (3)

130-130: Listing stacks without filters.

Using new ListStacksCommand({}) enumerates all stacks. This is fine, but filter usage might streamline the logic if needed.


200-200: Creating the cleanup stack.

This step ensures leftover resources are removed automatically. Consider logging any potential partial failures.


217-218: Describing base-stack resources.

Accessing this.baseStackName is consistent. As a small optimization, reuse the result if you already have this data from earlier calls.

src/model/cloud-runner/providers/aws/services/task-service.ts (5)

46-48: Consider adding error handling.

You’re calling CF.send(new ListStacksCommand({})) and immediately using its result without a try/catch or fallback. In the event of AWS-related network errors or throttling, the operation may fail.

+try {
  const stacks =
    (await CF.send(new ListStacksCommand({}))).StackSummaries?.filter(
      ...
    ) || [];
+} catch (error) {
+  CloudRunnerLogger.logError(`ListStacksCommand failed: ${error}`);
+  throw error;
+}

98-99: Reuse ECS client if possible.

Instantiating new ECS(...) repeatedly can be costly. Consider reusing a single instance of ECS if you’re calling it often in this file.


109-109: Error handling for DescribeTasks.

Like other AWS calls, consider wrapping ecs.send(new DescribeTasksCommand(...)) in a try/catch block to log or handle potential AWS errors gracefully.


159-159: Rename variable to avoid confusion.

You’re naming the CloudWatchLogs client as ecs. Rename it to something like logsClient or cwl for clarity.

-const ecs = new CloudWatchLogs();
+const logsClient = new CloudWatchLogs();
 ...
-let logGroupsDescribe = await ecs.send(new DescribeLogGroupsCommand(logStreamInput));
+let logGroupsDescribe = await logsClient.send(new DescribeLogGroupsCommand(logStreamInput));

Also applies to: 161-165


193-198: Make S3 calls more robust.

await s3.send(new ListObjectsCommand(listRequest)) can fail due to bucket permissions or region mismatch. Consider wrapping in try/catch for reliability.

src/model/cloud-runner/providers/aws/aws-task-runner.ts (4)

30-31: Refine static property names if desired.

Currently public static ECS and public static Kinesis are uppercase. Consider renaming them to ecsClient and kinesisClient for clarity, or making them private if scoping allows.


78-78: Consider wrapping RunTaskCommand in a try/catch.

In the event of an AWS service error, you can log additional info or retry as needed, rather than letting it bubble up unhandled.


190-190: Check for throttling or expired shard iterators.

When calling Kinesis.send(new GetRecordsCommand(...)), the code doesn’t explicitly handle potential throttling or an expired iterator. Consider backoff or error handling for resilience.


229-233: Consider memory usage for uncompressed logs.

Decompressing data via zlib.gunzipSync and parsing JSON might be large for high-volume logs. If log volumes grow, switch to a streaming / incremental approach.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d6bdcb and 12fed08.

⛔ Files ignored due to path filters (3)
  • dist/index.js is excluded by !**/dist/**
  • dist/index.js.map is excluded by !**/dist/**, !**/*.map
  • dist/licenses.txt is excluded by !**/dist/**
📒 Files selected for processing (9)
  • package.json (2 hunks)
  • src/model/cloud-runner/providers/aws/aws-base-stack.ts (4 hunks)
  • src/model/cloud-runner/providers/aws/aws-error.ts (1 hunks)
  • src/model/cloud-runner/providers/aws/aws-job-stack.ts (7 hunks)
  • src/model/cloud-runner/providers/aws/aws-task-runner.ts (9 hunks)
  • src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts (1 hunks)
  • src/model/cloud-runner/providers/aws/index.ts (4 hunks)
  • src/model/cloud-runner/providers/aws/services/garbage-collection-service.ts (5 hunks)
  • src/model/cloud-runner/providers/aws/services/task-service.ts (7 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/model/cloud-runner/providers/aws/services/garbage-collection-service.ts (1)
src/model/cloud-runner/providers/aws/services/task-service.ts (1)
  • TaskService (30-202)
src/model/cloud-runner/providers/aws/aws-error.ts (1)
src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
  • error (24-26)
src/model/cloud-runner/providers/aws/aws-base-stack.ts (1)
src/model/cloud-runner/providers/aws/cloud-formations/base-stack-formation.ts (1)
  • BaseStackFormation (1-397)
🪛 Biome (1.9.4)
src/model/cloud-runner/providers/aws/aws-base-stack.ts

[error] 60-60: Don't use 'Boolean' as a type.

Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead

(lint/complexity/noBannedTypes)

🔇 Additional comments (43)
package.json (1)

35-39: Good addition of AWS SDK v3 client packages.

The PR correctly adds the necessary AWS SDK v3 modular client packages that are needed for the migration. Each package targets a specific AWS service, which aligns with the modular approach of SDK v3.

src/model/cloud-runner/providers/aws/services/garbage-collection-service.ts (5)

1-8: Good practice using modular imports from AWS SDK v3.

You've correctly imported specific clients and commands from the AWS SDK v3 packages, which follows AWS best practices. This modular approach allows for better tree-shaking and smaller bundle sizes compared to importing the entire SDK.


22-24: Client instantiation has been properly updated.

The client instantiation has been correctly updated to the AWS SDK v3 pattern, properly passing the region in the constructor.


33-33: Command pattern implementation is correct.

You've correctly implemented the AWS SDK v3 command pattern by using the send method with a StopTaskCommand instance. This follows AWS's recommended approach for v3.


49-52: Good null check for CreationTime.

You've added a null check for element.CreationTime before passing it to the isOlderThan1day method, which prevents potential runtime errors. This is a good defensive programming practice.


60-61: TypeScript type safety improvement.

Adding the DeleteStackCommandInput type for deleteStackInput improves type safety and makes the code more maintainable. Good job implementing proper typing as part of the migration.

src/model/cloud-runner/providers/aws/aws-error.ts (3)

2-2: Correctly updated AWS SDK import.

You've properly updated the import statement to use the modular AWS SDK v3 format, importing only the needed components.


7-7: Parameter type correctly updated.

The parameter type for CF has been correctly updated from SDK.CloudFormation to CloudFormation, reflecting the new import structure.


12-12: Command pattern correctly implemented.

You've successfully migrated from the method-based approach with .promise() to the command pattern using send(new DescribeStackEventsCommand()). This follows AWS SDK v3 best practices.

src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts (2)

1-1: Updated import to use AWS SDK v3.

You've correctly updated the import statement to use the modular AWS SDK v3, importing only the StackResource type that's needed.


6-7:

✅ Verification successful

Types properly updated to AWS SDK v3 equivalents.

The property types have been correctly updated from AWS.CloudFormation.StackResources | undefined to Array<StackResource> | undefined, reflecting the type changes in AWS SDK v3.

One thing to verify is that all consumers of these properties are aware of the type change from a possibly undefined object to a possibly undefined array.


🏁 Script executed:

#!/bin/bash
# Search for any usage of taskDefResources or baseResources properties
rg -A 5 "taskDefResources|baseResources" --type ts

Length of output: 3625


Verified Type Compatibility for Array Properties

The type update to Array<StackResource> | undefined has been confirmed. Our search verifies that all consumers in files such as aws-task-runner.ts and aws-job-stack.ts invoke array methods (e.g., .find()) and correctly account for the possibility of the property being undefined. No further modifications are required.

src/model/cloud-runner/providers/aws/index.ts (4)

1-3: Good job migrating to AWS SDK v3 imports.

These modular imports are aligned with the PR goal of reducing bundle size and improving clarity. No issues here.


134-134: Method signature update is consistent with AWS SDK v3.

Switching from SDK.CloudFormation to CloudFormation is aligned with the v3 migration. Ensure all references throughout the codebase are updated accordingly.


136-136: Deleting main stack using command-based API.

Calling CF.send(new DeleteStackCommand(...)) directly is correct. Make sure you handle potential errors or race conditions if tasks are still running.


141-149: Waiting for main stack deletion.

Using waitUntilStackDeleteComplete with maxWaitTime: 200 seconds is helpful. For large or slow stacks, consider verifying if 200 seconds is sufficient.

src/model/cloud-runner/providers/aws/aws-base-stack.ts (13)

3-15: Switch to modular CloudFormation imports.

Great use of the new v3 command-based API. This makes the code more explicit and aligns with AWS best practices.


25-25: Updating method signature to accept v3 CloudFormation client.

This is consistent with the overall migration. Confirm that all callers now pass the v3 client.


31-31: Describing stacks using DescribeStacksCommandInput.

The new typed commands enhance type safety. No concerns here.


34-34: Parameter arrangement.

Properly structuring your stack parameters ensures clarity. Good approach in passing them as typed arrays.


39-39: Hash-based versioning.

Appending an MD5 hash is a neat way to handle stack updates. This avoids accidental drift.


43-44: Building update stack input.

Using typed inputs like UpdateStackCommandInput and referencing capabilities is aligned with v3’s design.


49-50: Building create stack input.

Same pattern. This approach keeps your code consistent and maintainable.


56-58: Listing existing stacks with filters.

Your filter logic for listing stacks is correct. The send + ListStacksCommand approach is standard for v3.


67-67: CreateStackCommand usage.

Clean approach to creating the base stack if it does not exist.


79-85: Waiting for stack creation with waitUntilStackCreateComplete.

This is the recommended approach in v3. Consider verifying if the 200-second wait is adequate for large stacks.


88-88: Log clarity.

Good job logging the existing stack version. This helps debug version mismatches.


92-92: Updating stack using new command-based API.

Make sure you handle the “no updates are to be performed” scenario gracefully, which you do.


113-118: Waiting for stack update to complete.

Same note about verifying the 200-second max wait time. Large updates may exceed that limit.

src/model/cloud-runner/providers/aws/aws-job-stack.ts (7)

1-9: AWS SDK v3 import statements.

This modular import pattern enhances maintainability and reduces code bloat.


27-27: Method signature updated to v3 client type.

This aligns with the overall migration. Ensure all references are updated across the code.


143-143: Defining createStackInput using v3 types.

This is consistent with the new command-based approach. Keep your parameter usage well-documented.


152-158: Waiting for the job stack to finish creation.

waitUntilStackCreateComplete is clear. Again, confirm that 200 seconds is enough time for all resources to stabilize.


159-159: Describing the newly created stack.

Verifying parameters post-creation is a wise guardrail to detect partial failures.


170-170: Creating cleanup stack input with v3 approach.

This is consistent with the job stack creation logic.


210-214: Describing stack resources for the task definition stack.

The command-based DescribeStackResourcesCommand is used correctly. Make sure you handle cases where resources might be in transitional states.

src/model/cloud-runner/providers/aws/services/task-service.ts (5)

1-23: Migration to AWS SDK v3 imports looks good.

These imports correctly reference the new modular packages from AWS SDK v3.


42-42: Generic type usage is correct.

Defining const result: StackSummary[] = [] ensures typed clarity for stack summaries.


55-57: Validate handling of undefined CreationTime.

Currently, if CreationTime is undefined, the code logs a message but continues using 0 as a fallback. This may skew the “age” calculation.

Would you like to skip these stacks altogether or handle them differently? Confirm the intended behavior when CreationTime is missing.

Also applies to: 59-60


94-94: LGTM on typed task result.

Defining result as { taskElement: Task; element: string }[] is clear and consistent.


122-122: Confirm logic for skipping tasks with no creation date.

If createdAt is missing, the code simply logs and continues. Verify that this behavior aligns with your requirements and doesn’t hide potential data issues.

src/model/cloud-runner/providers/aws/aws-task-runner.ts (3)

1-16: AWS SDK v3 imports look correct.

These updated imports for ECS and Kinesis align with the new command-based approach in v3.


126-132: Good usage of waiters.

Using waitUntilTasksRunning is a clean approach to block until ECS tasks are running. The try/catch with core.setFailed(error) is an appropriate errorflow.


257-263: Confirm shard iterator type.

Using 'TRIM_HORIZON' retrieves all historical records from the earliest point. Verify that you actually need from the beginning, rather than 'LATEST' to get only new logs.

Comment on lines +76 to +78
if (!element.CreationTime) {
CloudRunnerLogger.log(`${element.StackName} due to undefined CreationTime`);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Apply the same age calculation fix here as well.

For the base stacks, you have the same pattern for computing “age” using new Date(...). Apply the same fix for logical consistency.

Also applies to: 80-81

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/model/cloud-runner/providers/aws/services/task-service.ts (2)

135-163: Consider adding try/catch blocks to other AWS SDK calls.

You've added a try/catch block to the awsDescribeJob method, which is great. For consistency and robustness, consider adding similar error handling to other methods that make AWS SDK calls, like getCloudFormationJobStacks, getTasks, getLogGroups, and getLocks.

For example:

public static async getCloudFormationJobStacks() {
  const result: StackSummary[] = [];
  CloudRunnerLogger.log(``);
  CloudRunnerLogger.log(`List Cloud Formation Stacks`);
  process.env.AWS_REGION = Input.region;
  const CF = new CloudFormation({ region: Input.region });
+ try {
    const stacks =
      (await CF.send(new ListStacksCommand({}))).StackSummaries?.filter(
        (_x) =>
          _x.StackStatus !== 'DELETE_COMPLETE' && _x.TemplateDescription !== BaseStackFormation.baseStackDecription,
      ) || [];
    // rest of the method...
    return result;
+ } catch (error) {
+   CloudRunnerLogger.error(
+     `Failed to get CloudFormation job stacks: ${error instanceof Error ? error.message : String(error)}`,
+   );
+   throw error;
+ }
}

186-187: Optimize age calculation in getLogGroups method.

The age calculation in the getLogGroups method has the same issue as in other methods.

-const ageDate: Date = new Date(Date.now() - element.creationTime);
+const ageInMs: number = Date.now() - element.creationTime;
+const days = Math.floor(ageInMs / (1000 * 60 * 60 * 24));
+const hours = Math.floor((ageInMs % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60));
+const minutes = Math.floor((ageInMs % (1000 * 60 * 60)) / (1000 * 60));

Then update the logging in lines 188-192 to use these variables directly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12fed08 and db96d97.

⛔ Files ignored due to path filters (2)
  • dist/index.js is excluded by !**/dist/**
  • dist/index.js.map is excluded by !**/dist/**, !**/*.map
📒 Files selected for processing (1)
  • src/model/cloud-runner/providers/aws/services/task-service.ts (6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/model/cloud-runner/providers/aws/services/task-service.ts (2)
src/model/cloud-runner/providers/aws/cloud-formations/base-stack-formation.ts (1)
  • BaseStackFormation (1-397)
src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
  • error (24-26)
🔇 Additional comments (9)
src/model/cloud-runner/providers/aws/services/task-service.ts (9)

1-23: Nice job on modularizing the AWS SDK imports.

The change from a single aws-sdk import to modular imports from specific AWS SDK v3 packages is a great improvement. This approach results in smaller bundle sizes by only importing what's needed.


42-48: Good transition to command pattern.

The switch from promise-based calls to the command pattern is well implemented. The type change from StackSummaries to StackSummary[] also properly matches the v3 SDK return type.


55-60: Check for undefined CreationTime is a good addition, but age calculation could be improved.

The null check for CreationTime and the use of optional chaining with nullish coalescing is a great defensive programming practice. However, the age calculation method is not the most accurate way to calculate days.

-const ageDate: Date = new Date(Date.now() - (element.CreationTime?.getTime() ?? 0));
+const ageInMs: number = Date.now() - (element.CreationTime?.getTime() ?? 0);
+const days = Math.floor(ageInMs / (1000 * 60 * 60 * 24));
+const hours = Math.floor((ageInMs % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60));
+const minutes = Math.floor((ageInMs % (1000 * 60 * 60)) / (1000 * 60));

Then update the logging in lines 62-65 to use these variables directly.


76-80: Same age calculation issue for base stacks.

The same issue with age calculation exists here for base stacks.

-const ageDate: Date = new Date(Date.now() - (element.CreationTime?.getTime() ?? 0));
+const ageInMs: number = Date.now() - (element.CreationTime?.getTime() ?? 0);
+const days = Math.floor(ageInMs / (1000 * 60 * 60 * 24));
+const hours = Math.floor((ageInMs % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60));
+const minutes = Math.floor((ageInMs % (1000 * 60 * 60)) / (1000 * 60));

93-109: Good implementation of getTasks method with AWS SDK v3.

The type changes and command pattern implementation are well done. The method correctly handles the new AWS SDK v3 structure and maintains the original functionality.


133-164: Excellent error handling improvement in awsDescribeJob.

The addition of a try/catch block with specific error handling is a significant improvement. The error message includes the error type check to ensure the message is properly extracted, and the job ID is included for better debugging.


144-148: Same age calculation issue in awsDescribeJob.

The age calculation has the same issue as noted earlier.

-const ageDate: Date = new Date(Date.now() - (stack.CreationTime?.getTime() ?? 0));
+const ageInMs: number = Date.now() - (stack.CreationTime?.getTime() ?? 0);
+const days = Math.floor(ageInMs / (1000 * 60 * 60 * 24));
+const hours = Math.floor((ageInMs % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60));
+const minutes = Math.floor((ageInMs % (1000 * 60 * 60)) / (1000 * 60));

Then update the message in lines 148-154 to use these variables directly.


165-177: Well-structured update for getLogGroups method.

The changes to the getLogGroups method are well-implemented. The type change from LogGroups to Array<LogGroup> and the introduction of the command pattern are appropriate for the AWS SDK v3 migration.


198-205: Nicely implemented getLocks method update.

The S3 client instantiation and command pattern conversion are well done.

Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@webbertakken
Copy link
Member

If someone can verify that this works, we should be good to merge it.

@codecov
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 13.76147% with 94 lines in your changes missing coverage. Please review.

Project coverage is 37.12%. Comparing base (9d6bdcb) to head (bb2bf25).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...loud-runner/providers/aws/services/task-service.ts 8.88% 41 Missing ⚠️
...model/cloud-runner/providers/aws/aws-base-stack.ts 7.69% 12 Missing ⚠️
...odel/cloud-runner/providers/aws/aws-task-runner.ts 14.28% 12 Missing ⚠️
...oviders/aws/services/garbage-collection-service.ts 23.07% 10 Missing ⚠️
src/model/cloud-runner/providers/aws/index.ts 25.00% 9 Missing ⚠️
.../model/cloud-runner/providers/aws/aws-job-stack.ts 11.11% 8 Missing ⚠️
src/model/cloud-runner/providers/aws/aws-error.ts 33.33% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
+ Coverage   37.04%   37.12%   +0.07%     
==========================================
  Files          77       77              
  Lines        3131     3157      +26     
  Branches      647      623      -24     
==========================================
+ Hits         1160     1172      +12     
- Misses       1828     1985     +157     
+ Partials      143        0     -143     
Files with missing lines Coverage Δ
src/model/cloud-runner/providers/aws/aws-error.ts 41.66% <33.33%> (+5.30%) ⬆️
.../model/cloud-runner/providers/aws/aws-job-stack.ts 13.63% <11.11%> (+1.32%) ⬆️
src/model/cloud-runner/providers/aws/index.ts 17.64% <25.00%> (+2.49%) ⬆️
...oviders/aws/services/garbage-collection-service.ts 14.58% <23.07%> (+3.21%) ⬆️
...model/cloud-runner/providers/aws/aws-base-stack.ts 10.71% <7.69%> (+1.62%) ⬆️
...odel/cloud-runner/providers/aws/aws-task-runner.ts 11.20% <14.28%> (+1.38%) ⬆️
...loud-runner/providers/aws/services/task-service.ts 8.40% <8.88%> (+1.79%) ⬆️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@frostebite
Copy link
Member

Amazing, looks great!

Copy link
Member

@frostebite frostebite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very pleased to have this improvement. Thanks. The testing steps you mention are also helpful. Lgtm.

@webbertakken webbertakken merged commit 81ed299 into game-ci:main Apr 10, 2025
5 checks passed
@webbertakken
Copy link
Member

Released in v4.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants