chore: update task run to use manager roles and update manager permissions#1212
Conversation
bvtujo
left a comment
There was a problem hiding this comment.
Looks good to me. Just a small nit.
iamhopaul123
left a comment
There was a problem hiding this comment.
Looks pretty good to me! Just one tiny nit 
|
|
||
| opts := &runTaskOpts{ | ||
| runTaskVars: runTaskVars{ | ||
| GlobalOpts: &GlobalOpts{}, |
There was a problem hiding this comment.
nit: can we remove this?
There was a problem hiding this comment.
no because we need it to call AppName() in deploy() 😢
| if opts.env != "" { | ||
| opts.deployOpts = []awscloudformation.StackOption{awscloudformation.WithRoleARN("execution role")} | ||
| } |
There was a problem hiding this comment.
Mmm we shouldn't have custom logic in our tests otherwise we are not testing the real code-path.
I wonder if instead we should store the targetEnv as a field and have that lookup in the deploy() method so that we test if the real code checks if an env is not nil and passes the appropriate execution role arn.
I really like the two test-cases written above :)
There was a problem hiding this comment.
If targetEnv is being set in configureRuntimeOpts, it seems like we will still not able to set it without writing custom logic in the tests, right?
It seems to me that in order to make the testing work, I will have to somehow isolate the targetEnv-related logic from configureRuntimeOpts.
This is reflected in the latest code change. I'm not sure if I like this approach - please let me know what you think👀
There was a problem hiding this comment.
jes totally makes sense, thank you!
| func (o *runTaskOpts) configureSessAndEnv() error { | ||
| var sess *awssession.Session | ||
| var env *config.Environment | ||
| var err error |
f1e629d to
7e5ca73
Compare
This PR contains the following changes: 1. implement `--follow` flag for `task run` 2. implement `EarliestStartTime` method in `task` package in support of `--follow` 3. wrap `aws-sdk-go/ecs` constant `DesiredStatusStopped` in `ecs` package in support of `--follow` Should be merged only after #1212 Related #702 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
This PR contains the following changes:
task runtask runto use environment managerBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.