Skip to content

Conversation

turingbeing
Copy link

@turingbeing turingbeing commented Jun 19, 2024

Adds a new conditional feature to set previous task definitions to INACTIVE, currently old definitions are left ACTIVE. The feature is optionally controlled via the deregister-tasks flag and is false by default.

Adds:

  • Conditional to control Task Deregistration
  • deregister_old_task_definitions function in plugin.bash

Updates:

  • Success logic to remove old tasks if enabled
  • README
  • Tests

Copy link
Contributor

@toote toote left a comment

Choose a reason for hiding this comment

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

great PR!

left a few comments here and there about code organization and execution flow

--output text)

# Array
readarray -t active_task_defs <<<"$all_active_task_defs"
Copy link
Contributor

Choose a reason for hiding this comment

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

the readarray/mapfiles function can not be assumed to exist in all environments so I'd suggest an alternative

Comment on lines +73 to +78
# Remove the current task definition from the list
for i in "${!active_task_defs[@]}"; do
if [[ "${active_task_defs[$i]}" == *":${task_revision}"* ]]; then
unset 'active_task_defs[$i]'
fi
done
Copy link
Contributor

Choose a reason for hiding this comment

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

looping through all active definitions to remove a single element may be a bad idea... I'd suggest doing this check in the next loop while removing them

Copy link
Contributor

Choose a reason for hiding this comment

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

this file is for plugin-specific functionality, I'd suggest creating the function in the main file or creating a new library file for these kind of things

Comment on lines +65 to +68
all_active_task_defs=$(aws ecs list-task-definitions \
--family-prefix "${task_family}" \
--query 'taskDefinitionArns[]' \
--output text)
Copy link
Contributor

Choose a reason for hiding this comment

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

please make sure to re-use the default options to aws commands like it is done on the hook file because it may include regions without which the commands will probably fail

Comment on lines +81 to +84
for task_def in "${active_task_defs[@]}"; do
echo "Deregistering $task_def"
aws ecs deregister-task-definition --task-definition "$task_def"
done
Copy link
Contributor

Choose a reason for hiding this comment

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

please make sure to re-use the default options to aws commands like it is done on the hook file because it may include regions without which the commands will probably fail

@turingbeing
Copy link
Author

Thanks for the feedback @toote I'll make some changes, currently enjoying some holiday, so might be another week yet 👍

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.

2 participants