Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[semver:minor] Add support for tagging of task-definition to update-service command #127

Merged
merged 5 commits into from
Apr 16, 2021
Merged

[semver:minor] Add support for tagging of task-definition to update-service command #127

merged 5 commits into from
Apr 16, 2021

Conversation

dgeorges
Copy link
Contributor

@dgeorges dgeorges commented Apr 7, 2021

Fix for #120

If the task-definition-tags parameter is specified for the update-service command. The task definition will be updated with the tag specified.

@dgeorges dgeorges changed the title [semver:minor] Add support for tagging of task-def to update-service command [semver:minor] Add support for tagging of task-definition to update-service command Apr 7, 2021
@dgeorges
Copy link
Contributor Author

dgeorges commented Apr 9, 2021

@KyleTryon Would you be able to review this change?
my apologies if pinging you directly is not the correct process, but it's unclear how the review process works for external contributors and given your most recent rewrite PR you seem to be the most appropriate reviewer. :)

@KyleTryon KyleTryon self-assigned this Apr 12, 2021
@KyleTryon
Copy link
Contributor

Thanks for the ping @dgeorges. This looks good 👍 . Could I ask you to add the same parameter to the jobs which use this command as well and pass-through the values?

https://github.com/CircleCI-Public/aws-ecs-orb/blob/master/src/jobs/deploy-service-update.yml

Also, are you aware of any issues with using the --tags flag with no values? The tests so far have appeared to pass.

@KyleTryon KyleTryon self-requested a review April 12, 2021 17:08
Copy link
Contributor

@KyleTryon KyleTryon left a comment

Choose a reason for hiding this comment

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

@dgeorges
Copy link
Contributor Author

Thanks for the ping @dgeorges. This looks good 👍 . Could I ask you to add the same parameter to the jobs which use this command as well and pass-through the values?

https://github.com/CircleCI-Public/aws-ecs-orb/blob/master/src/jobs/deploy-service-update.yml

Also, are you aware of any issues with using the --tags flag with no values? The tests so far have appeared to pass.

The --tags flag is required. My testing showed it throws the parsing error below if empty which is protected against with the when clause. A similar error is returned if the value isn't formatted correctly.
Error parsing parameter '--tags': Expected: '=', received: 'EOF' for input:

FYI update the documentation for the JSON syntax as well, which I tested.

@dgeorges dgeorges requested a review from KyleTryon April 12, 2021 19:45
Copy link
Contributor

@KyleTryon KyleTryon left a comment

Choose a reason for hiding this comment

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

Glorious 🎉 thank you for your contribution. Testing one last time and merging.

@dgeorges
Copy link
Contributor Author

awesome! Thanks for the review

@KyleTryon KyleTryon merged commit 14f3c4e into CircleCI-Public:master Apr 16, 2021
@dgeorges
Copy link
Contributor Author

@KyleTryon Looks like the circleci pipeline timed out on the codedeploy_fargate_tear-down-test-env job of the integration-test_deploy workflow while doing a terraform destroy operation. It passed on the branch though and failed on master about 2 months ago too. So looks like an aws/terraform timing thing and not related to the PR. I think a retry will be sufficient, if you wouldn't mind kicking that off, I don't have permissions.

@lokst
Copy link
Contributor

lokst commented Apr 21, 2021

@dgeorges Thanks for alerting us to the failed build. I've rerun the workflow and your changes have been published as circleci/aws-ecs@2.2.0. Thank you for your patience on this!

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.

None yet

3 participants