Add profile-name option to update-service orb #60

lucasnad27 · 2019-10-11T01:13:07Z

Checklist

All new jobs, commands, executors, parameters have descriptions
Examples have been added for any significant new features
README has been updated, if necessary

Motivation, issues

Please see #41 for detailed information on this patch

Description

add optional profile-name parameter to deploy-service-update

Add newline escapes Use `default` profile instead of env var

lokst · 2019-10-11T02:14:31Z

src/orb.yml.hbs

+        description:
+          AWS profile used for calls to aws api.
+        type: string
+        default: default


Could we make it "" by default? Ideally, I would like --profile to not be set when the value of profile-name is "", to avoid conflicts with existing usage of the orb.

Could you update .circleci/config.yml to add profile-name: default to the aws-ecs/deploy-service-update job at

aws-ecs-orb/.circleci/config.yml

Line 575 in 8626489

- aws-ecs/deploy-service-update:

and

aws-ecs-orb/.circleci/config.yml

Line 806 in 8626489

- aws-ecs/deploy-service-update:

to include the new parameter in our integration tests?

lokst · 2019-10-11T02:15:49Z

src/orb.yml.hbs

@@ -564,17 +570,20 @@ commands:
                    --cluster << parameters.cluster-name >> \
                    --services ${SERVICE_NAME} \
                    --output text \
-                    --query 'services[0].deployments[].[taskDefinition, status]')
+                    --query 'services[0].deployments[].[taskDefinition, status]' \
+                    --profile << parameters.profile-name >>)


I don't think this will work as expected as the profile-name parameter should also be defined in the command and the job should pass the parameter to the command. Additionally, I would like it if --profile were not set if the value of profile-name is "".

aws-cli-orb creates the default profile. Couldn't we follow the same pattern ? @lokst can you explain your concern about having a profile name empty please ?

@sylwit I'd like to avoid the chance of unintended effects on users as the orb's commands may not necessarily be used with the aws-cli orb. Hence, I'd rather --profile not be set if the user did not specify a non-empty value.

lokst · 2019-10-11T02:17:55Z

src/orb.yml.hbs

                NUM_DEPLOYMENTS=$(aws ecs describe-services \
                    --cluster << parameters.cluster-name >> \
                    --services ${SERVICE_NAME} \
                    --output text \
-                    --query 'length(services[0].deployments)')
+                    --query 'length(services[0].deployments)' \
+                    --profile << parameters.profile-name >>)


Could this be updated such that --profile will not be set if the value of profile-name is ""?

lokst · 2019-10-11T02:18:00Z

src/orb.yml.hbs

                TARGET_REVISION=$(aws ecs describe-services \
                    --cluster << parameters.cluster-name >> \
                    --services ${SERVICE_NAME} \
                    --output text \
-                    --query "services[0].deployments[?taskDefinition==\`<< parameters.task-definition-arn >>\` && runningCount == desiredCount && (status == \`PRIMARY\` || status == \`ACTIVE\`)][taskDefinition]")
+                    --query "services[0].deployments[?taskDefinition==\`<< parameters.task-definition-arn >>\` && runningCount == desiredCount && (status == \`PRIMARY\` || status == \`ACTIVE\`)][taskDefinition]" \
+                    --profile << parameters.profile-name >>)


Could this be updated such that --profile will not be set if the value of profile-name is ""?

lokst · 2019-10-11T02:18:35Z

src/orb.yml.hbs

@@ -619,7 +628,7 @@ commands:
      - run:
          name: Retrieve previous task definition and prepare new task definition values
          command: |
-            PREVIOUS_TASK_DEFINITION=$(aws ecs describe-task-definition --task-definition << parameters.family >> --include TAGS)
+            PREVIOUS_TASK_DEFINITION=$(aws ecs describe-task-definition --task-definition << parameters.family >> --profile << parameters.profile-name >> --include TAGS)


I don't think this will work as expected as the profile-name parameter should also be defined in the command and the job should pass the parameter to the command. Additionally, I would like it if --profile were not set if the value of profile-name is "".

lokst · 2019-10-11T02:19:39Z

src/orb.yml.hbs

@@ -716,7 +725,9 @@ commands:
                --container-definitions "${CCI_ORB_AWS_ECS_CONTAINER_DEFS}" \
                "$@" \
                --output text \
-                --query 'taskDefinition.taskDefinitionArn')
+                --query 'taskDefinition.taskDefinitionArn' \
+                --profile << parameters.profile-name >>)


Could this be updated such that --profile will not be set if the value of profile-name is ""?

lokst

@lucasnad27 Thank you for your awesome work! I left some comments and would love if you could review them!

lucasnad27 · 2019-10-11T06:28:10Z

I had similar reservations about making default the....default :)

You can expect a follow up commit this weekend. Thanks for the quick turnaround on the review!

lucasnad27 · 2019-10-23T21:45:27Z

Not quite "next weekend" Primarily due to social obligations and some re-prioritization at work, but...

@lokst I wanted to get your take on this before I spend any more time getting the arguments to parse correctly. The most elegant way I could find to do this in bash (disclaimer: my bash skills are decidedly NOT elegant) was to build up an array with conditionals...

params=(
    "--cluster staging"
    "--services public"
    "--output text"
    "--query 'services[0].deployments[].[taskDefinition, status]'")
if [ "$PROFILE" ]; then
    params+=("--profile ${PROFILE}")
fi

$(aws ecs describe-services "${params[@]}")

This almost works but I'm getting a jmespath error (Bad jmespath expression: Unclosed ' delimiter, hence my hesitance to debug this any further until I have a sanity check on the best way to accomplish this. As you've noted in your comments above, I'll have to change a pretty good chunk of the aws-cli commands with this tactic so getting some buy-in would be ideal :) Thanks again!

lokst · 2019-10-29T05:33:20Z

@lucasnad27 No worries and thanks so much for taking the time! 🙂

Regarding the approach, for consistency, I think we could use something similar to what is done in other parts of the orb, for optional parameters:

aws-ecs-orb/src/orb.yml.hbs

Lines 680 to 682 in bff006f

    
           if [ -n "${CCI_ORB_AWS_ECS_TASK_ROLE}" ]; then 
        
               set -- "$@" --task-role-arn "${CCI_ORB_AWS_ECS_TASK_ROLE}" 
        
           fi

and

aws-ecs-orb/src/orb.yml.hbs

Line 719 in bff006f

"$@" \

(If there are cases where "$@" is used for more than one AWS command within a step, the positional arguments can be cleared with set -- before being set for the next AWS command)

sylwit · 2019-11-05T02:38:17Z

I was working on the same "feature" when I found that you already have this PR. I'll be happy to help if you need @lucasnad27

lucasnad27 · 2019-11-12T04:51:47Z

Thanks for the guidance @lokst on preferred way to implement this the feature. I spoke with @sylwit offline and one of us will get this PR fixed up soon :)

Add profile-name option to update-service orb

080ad6b

Add newline escapes Use `default` profile instead of env var

lucasnad27 requested review from iynere and lokst as code owners October 11, 2019 01:13

lokst reviewed Oct 11, 2019

View reviewed changes

lokst suggested changes Oct 11, 2019

View reviewed changes

lokst added the ignore_sla label Nov 8, 2019

lucasnad27 closed this Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add profile-name option to update-service orb #60

Add profile-name option to update-service orb #60

lucasnad27 commented Oct 11, 2019 •

edited

Loading

lokst Oct 11, 2019

lokst Oct 11, 2019

lokst Oct 11, 2019 •

edited

Loading

sylwit Nov 27, 2019

lokst Nov 27, 2019

lokst Oct 11, 2019

lokst Oct 11, 2019

lokst Oct 11, 2019

lokst Oct 11, 2019

lokst left a comment

lucasnad27 commented Oct 11, 2019

lucasnad27 commented Oct 23, 2019

lokst commented Oct 29, 2019

sylwit commented Nov 5, 2019

lucasnad27 commented Nov 12, 2019

Add profile-name option to update-service orb #60

Add profile-name option to update-service orb #60

Conversation

lucasnad27 commented Oct 11, 2019 • edited Loading

Checklist

Motivation, issues

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokst Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokst left a comment

Choose a reason for hiding this comment

lucasnad27 commented Oct 11, 2019

lucasnad27 commented Oct 23, 2019

lokst commented Oct 29, 2019

sylwit commented Nov 5, 2019

lucasnad27 commented Nov 12, 2019

lucasnad27 commented Oct 11, 2019 •

edited

Loading

lokst Oct 11, 2019 •

edited

Loading