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

feature(run-task): add run-task command #35

Merged
merged 6 commits into from
Oct 6, 2019
Merged

feature(run-task): add run-task command #35

merged 6 commits into from
Oct 6, 2019

Conversation

codingdiaz
Copy link
Contributor

@codingdiaz codingdiaz commented Sep 29, 2019

#10

This wraps aws ecs run-task with basic logic. I would love to work to get this merged (can update change / write tests etc.) I know of a couple teams that also want this functionality.

I can also add examples.

@codingdiaz codingdiaz changed the title feature(run-task): add run-task command used for running a single ECS… feature(run-task): add run-task command Sep 29, 2019
@lokst
Copy link
Contributor

lokst commented Sep 30, 2019

Thanks @codingdiaz, I'm excited for this! Could you add a job that wraps this command too? Would you be able to also add an example for this under examples in src/orb.yml.hbs ?

@lokst lokst self-assigned this Sep 30, 2019
@codingdiaz
Copy link
Contributor Author

@lokst of course. I’ll do that today.

I also didn’t know if an environment variable should be set or if I should handle any of the output from the run command. There are some times where the command runs fine but a task is never launched.

I also was unsure how to implement polling of a task. I was thinking just a job that gets the status every 30 seconds or so. What do you think?

@codingdiaz
Copy link
Contributor Author

@lokst I updated this PR to include a run-task job and two examples for running a fargate task and an ec2 task. Let me know if anything needs to be modified / added etc.

I was slightly unsure what to put for the version of this orb in my examples since it will be a future release. I was unsure if automation updates that on new releases.

src/orb.yml.hbs Outdated
task-definition: "myapp"
awsvpc: true
subnet-ids: "subnet-70faa93b,subnet-bcc54b93"
security-group-ids: "sg-010a460f7f442fa75"

Choose a reason for hiding this comment

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

🤔 Surely both of these values should be stubbed out and be required to have "real" values injected at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm what do you mean @sgerrand ?

Choose a reason for hiding this comment

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

👋 Apologies. I should have been clearer in my previous comment.

I meant that the values for the keys in this step (e.g. subnet-ids and security-group-ids) are using "real" values. Other examples in this file are using variables for substitution–perhaps this should use the same approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean for example:

subnet-ids: $SUBNET_ONE, $SUBNET_TWO

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the real values are helpful as an example of what typical values would look like. Perhaps something like this would be nice?

# e.g. "subnet-70faa93b,subnet-bcc54b93"
subnet-ids: $SUBNET_ONE, $SUBNET_TWO
# e.g. "sg-010a460f7f442fa75"
security-group-ids: $SECURITY_GROUP_IDS

@lokst
Copy link
Contributor

lokst commented Oct 1, 2019

@codingdiaz Thanks for the updates! For the examples you could also use circleci/aws-ecs@x.y.z since the version number of the release is unclear. I would like to review your changes asap, but as I'll be away most of the week, I may not be able to get to it till next week.

src/orb.yml.hbs Outdated Show resolved Hide resolved
src/orb.yml.hbs Outdated Show resolved Hide resolved
src/orb.yml.hbs Outdated Show resolved Hide resolved
src/orb.yml.hbs Outdated Show resolved Hide resolved
src/orb.yml.hbs Outdated Show resolved Hide resolved
Copy link
Contributor

@lokst lokst left a comment

Choose a reason for hiding this comment

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

@codingdiaz This looks great! Could you review my suggestions?

@codingdiaz
Copy link
Contributor Author

@codingdiaz This looks great! Could you review my suggestions?

Hey @lokst I incorporated those comment, thanks! Anything else that should be changed for this PR?

Copy link
Contributor

@lokst lokst left a comment

Choose a reason for hiding this comment

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

@codingdiaz This looks almost ready to be merged! Could you review my comment and also merge the latest changes from master again?

src/orb.yml.hbs Outdated Show resolved Hide resolved
@lokst lokst added the enhancement New feature or request label Oct 6, 2019
@lokst lokst merged commit a2a4fe9 into CircleCI-Public:master Oct 6, 2019
@lokst lokst added this to the 0.0.13 milestone Oct 6, 2019
@lokst
Copy link
Contributor

lokst commented Oct 6, 2019

@codingdiaz Your changes are now published in circleci/aws-ecs@0.0.13 ! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants