Conversation
|
ECS config (default region set to eu-west-2) Listing available task definitions in default region and diferent region (us-west-2): Local up with task def name from different region than configured region: With task def name that exists in region configured: With ARN of task definition in different region than configured region |
|
Not quite sure how best to unit test this, since the relevant changes would involve calling a mock client. |
efekarakus
left a comment
There was a problem hiding this comment.
Looks good to me! Just two small questions
| } | ||
|
|
||
| commandConfig, err := newCommandConfig(p.context, rdwr) | ||
| commandConfig, err := newCommandConfig(p.context, rdwr, "") |
There was a problem hiding this comment.
We can get rid of getSecret() right? I don't think this function is used anymore 🤔
There was a problem hiding this comment.
yes -- also looking at your changes from #808 we can probably get rid of the GetSecretValue method in client method -- I'll remove them in a separate PR.
| ecsConfig.Cluster = clusterFromEnv | ||
| // Determine Cloudformation StackName | ||
| if ecsConfig.Version == iniConfigVersion { | ||
| ecsConfig.CFNStackName = ecsConfig.CFNStackNamePrefix + ecsConfig.Cluster |
There was a problem hiding this comment.
Why did we add this? I don't think this check was there before right
There was a problem hiding this comment.
This was for backwards compatibility for the configure workflows that used the old config file format. /c @PettitWesley
Previously, passing a full task definition ARN to
local upwould not account for the region specified in the ARN and would default to the region configuration of the ECS client. This change enables local up to fetch and run a task definition from the correct region specified in its ARN.Fixes #821
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Documentation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.