Skip to content

ECS Run Tasks and Update Images#130

Merged
kferrone merged 8 commits intomainfrom
run-ecs-task
Apr 4, 2025
Merged

ECS Run Tasks and Update Images#130
kferrone merged 8 commits intomainfrom
run-ecs-task

Conversation

@kferrone
Copy link
Copy Markdown
Collaborator

@kferrone kferrone commented Mar 18, 2025

User description

Describe Changes

Run ecs tasks from the cli.

Link to Issues

https://app.clickup.com/t/8655600/DUPLO-30502

PR Review Checklist

  • Thoroughly reviewed on local machine.
  • Have you added any tests
  • Make sure to note changes in Changelog

PR Type

Enhancement, Tests


Description

  • Added new ECS service management commands, including run_task, delete_service, and list_tasks.

  • Enhanced task definition handling with prefixed_name utility and updated endpoint paths.

  • Introduced test data files for ECS services and task definitions.

  • Updated submodule reference in the wiki file.


Changes walkthrough 📝

Relevant files
Enhancement
ecs_service.py
Added ECS service and task management commands                     

src/duplo_resource/ecs_service.py

  • Added commands for managing ECS services and tasks (run_task,
    delete_service, list_tasks, etc.).
  • Enhanced task definition handling with prefixed_name and updated
    endpoint paths.
  • Improved error handling and messaging for service updates.
  • +94/-22 
    args.py
    Added new argument definitions for ECS commands                   

    src/duplocloud/args.py

    • Added new argument definitions for ECS commands.
    +2/-0     
    resource.py
    Added utility for ECS naming and endpoint updates               

    src/duplocloud/resource.py

  • Added prefixed_name utility for consistent ECS naming conventions.
  • Updated endpoint method for improved path handling.
  • +9/-0     
    Tests
    ecs_service.yaml
    Added test data for ECS service configurations                     

    src/tests/data/ecs_service.yaml

    • Added test data for ECS service configurations.
    +25/-0   
    ecs_td.yaml
    Added test data for ECS task definitions                                 

    src/tests/data/ecs_td.yaml

    • Added test data for ECS task definitions.
    +48/-0   
    Miscellaneous
    wiki
    Updated submodule reference in `wiki`                                       

    wiki

    • Updated submodule reference for the wiki file.
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @zafarabbas
    Copy link
    Copy Markdown
    Contributor

    Task linked: DUPLO-30502 Run ECS Tasks

    @duploctl
    Copy link
    Copy Markdown
    Contributor

    duploctl Bot commented Mar 18, 2025

    ☂️ Python Coverage

    current status: ✅

    Overall Coverage

    Lines Covered Coverage Threshold Status
    2568 546 21% 0% 🟢

    New Files

    No new covered files...

    Modified Files

    File Coverage Status
    src/duplo_resource/ecs_service.py 0% 🟢
    src/duplocloud/args.py 100% 🟢
    src/duplocloud/resource.py 39% 🟢
    TOTAL 46% 🟢

    updated for commit: bcd89d4 by action🐍

    new run task method
    update image will update task defs and services if they are attached
    @kferrone kferrone changed the title run and wait on ecs tasks ECS Run Tasks and Update Images Mar 26, 2025
    @kferrone kferrone marked this pull request as ready for review March 26, 2025 14:51
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The run_task method doesn't handle potential errors when the task fails to start. Consider adding more robust error handling for task execution failures.

    def run_task(self, 
                 name: args.NAME,
                 replicas: args.REPLICAS,
                 wait: args.WAIT) -> dict:
      """Run a task for an ECS service."
    
      Execute a task based on some definition. 
    
      Args:
        name: The name of the ECS service to run the task for.
        replicas: The number of replicas to run.
        wait: Whether to wait for the task to complete.
      Returns:
        message: A message indicating the task has been run.
      """
      td = self.find_def(name)
      tenant_id = self.tenant["TenantId"]
      path = f"v3/subscriptions/{tenant_id}/aws/runEcsTask"
      body = {
        "TaskDefinition": td["TaskDefinitionArn"], 
        "Count": replicas if replicas else 1
      }
      res = self.duplo.post(path, body)
      if wait:
        self.wait(lambda: self.wait_on_task(name))
      return res.json()
    Commented Code

    There's a comment on line 187 that appears to be a development note rather than documentation. This should be removed or converted to proper documentation.

    # erm, these errors should bubble up but they are probably getting suppressed because the two previous lines should be suppressed, however this line can only happen if there is actually a service
    self.update_service(svc)
    Wait Mechanism

    The wait_on_task method raises an exception when tasks are still running, which seems like an unusual pattern for a wait function. Consider using a more standard approach for waiting.

    @Command()
    def wait_on_task(self,
                     name: args.NAME) -> None: 
      """Wait for an ECS task to complete."""
      tasks = self.list_tasks(name)
      # filter the tasks down to any where the DesiredStatus and LastStatus are different
      running_tasks = [t for t in tasks if t["DesiredStatus"] != t["LastStatus"]]
      if len(running_tasks) > 0:
        raise DuploError(f"Service {name} waiting for replicas update", 400)

    Copy link
    Copy Markdown

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR introduces new CLI commands to manage ECS services and task definitions, along with corresponding test data definitions.

    • Adds new commands: list_services, find_service_family, delete_service, update_service, run_task, and wait_on_task.
    • Updates existing service and task definition lookup flows to use a prefixed naming scheme and improved endpoint handling.
    • Adds YAML test definitions for ECS task definitions and ECS services.

    Reviewed Changes

    Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

    Show a summary per file
    File Description
    src/tests/data/ecs_td.yaml Provides test data for ECS task definitions.
    src/tests/data/ecs_service.yaml Provides test data for ECS services.
    src/duplo_resource/ecs_service.py Updates and adds new CLI commands for handling ECS resources.
    src/duplocloud/resource.py Adds the helper method prefixed_name for resource name prepending.
    src/duplocloud/args.py Minor formatting changes in argument definitions.
    Files not reviewed (1)
    • wiki: Language not supported

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Mar 26, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix wait method behavior
    Suggestion Impact:The commit partially implements the suggestion's intent by modifying the wait_on_task method. While it doesn't change the return type to bool as suggested, it does remove the Command decorator and change the parameter type from args.NAME to str, indicating it's now meant to be used as an internal helper function rather than a command. This aligns with the suggestion's intent to make it work better in wait loops.

    code diff:

    -  @Command()
       def wait_on_task(self,
    -                   name: args.NAME) -> None: 
    +                   name: str) -> None: 

    The wait_on_task method raises an error when tasks are still running, but this
    is unexpected behavior for a wait method. Instead, it should return a boolean
    indicating whether tasks are still running, which would allow the wait loop to
    continue properly.

    src/duplo_resource/ecs_service.py [266-274]

     @Command()
     def wait_on_task(self,
    -                 name: args.NAME) -> None: 
    -  """Wait for an ECS task to complete."""
    +                 name: args.NAME) -> bool: 
    +  """Wait for an ECS task to complete.
    +  
    +  Returns:
    +    bool: True if tasks are complete, False if still running
    +  """
       tasks = self.list_tasks(name)
       # filter the tasks down to any where the DesiredStatus and LastStatus are different
       running_tasks = [t for t in tasks if t["DesiredStatus"] != t["LastStatus"]]
    -  if len(running_tasks) > 0:
    -    raise DuploError(f"Service {name} waiting for replicas update", 400)
    +  return len(running_tasks) == 0

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    __

    Why: This suggestion identifies a critical logical issue in the wait_on_task method. The current implementation raises an error when tasks are still running, which breaks the expected behavior of a wait method. The fix properly changes the return type and behavior to support the wait loop pattern.

    High
    General
    Fix unclosed docstring quote

    Fix the unclosed double quote in the docstring. The first line of the docstring
    has an unclosed double quote which can cause issues with documentation
    generation and may confuse other developers.

    src/duplo_resource/ecs_service.py [243-253]

     @Command()
     def run_task(self, 
                  name: args.NAME,
                  replicas: args.REPLICAS,
                  wait: args.WAIT) -> dict:
    -  """Run a task for an ECS service."
    +  """Run a task for an ECS service.
     
       Execute a task based on some definition. 
     
       Args:
         name: The name of the ECS service to run the task for.
         replicas: The number of replicas to run.
         wait: Whether to wait for the task to complete.
       Returns:
         message: A message indicating the task has been run.
       """

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies an unclosed double quote in the docstring which could cause issues with documentation generation. This is a minor but important fix for code quality and documentation correctness.

    Low
    Fix grammatical error
    Suggestion Impact:The commit implemented the exact grammatical correction suggested, changing 'it's' to 'its' in the message string on line 16 of the diff

    code diff:

    -    msg = "Updating a task definition and it's corresponding service."
    +    msg = "Updating a task definition and its corresponding service."

    Fix the grammatical error in the message string. "it's" is the contraction of
    "it is", but the possessive form "its" is needed here. This improves code
    readability and professionalism.

    src/duplo_resource/ecs_service.py [182]

     def update_image(self,
                      name: args.NAME,
                      image: args.IMAGE):
       """Update the image for an ECS service.
     
       Args:
         name: The name of the ECS service to update.
         image: The new image to use.
       Returns:
         ecs: The updated ECS object.
       Raises:
           DuploError: If the ECS service could not be updated.
       """
       name = self.prefixed_name(name)
       tdf = self.find_def(name) 
       tdf["ContainerDefinitions"][0]["Image"] = image
       arn = self.update_taskdef(tdf)["arn"]
    -  msg = "Updating a task definition and it's corresponding service."
    +  msg = "Updating a task definition and its corresponding service."

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 3

    __

    Why: The suggestion correctly identifies a grammatical error ("it's" vs "its") in a message string. While this is a minor issue that doesn't affect functionality, fixing it improves code readability and professionalism.

    Low
    • Update

    kferrone and others added 3 commits March 26, 2025 08:54
    Comment thread src/duplo_resource/ecs_service.py
    Comment thread src/duplo_resource/ecs_service.py
    @kferrone kferrone merged commit 9cccb8c into main Apr 4, 2025
    5 checks passed
    @kferrone kferrone deleted the run-ecs-task branch April 4, 2025 16:03
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    5 participants