feat(ecs): support selecting multiple subnet types #8724
Conversation
|
Currently Testing (will remove do-not-merge label once ready). |
There was a problem hiding this comment.
Couple questions, including this one:
- is there anywhere in deck we need to make changes in order to correctly display multiple subnets on a service? (like cluster/LB views - these might only use VPC but want to confirm)
also can you add some test output/screenshots of:
- the multi-select for subnetTypes working
- a successful deployment w/ multiple subnets (ECS console)
- a validation error when both
subnetTypeandsubnetTypesare used?
| @@ -336,7 +337,7 @@ export class EcsServerGroupConfigurationService { | |||
| .filter({ | |||
| account: command.credentials, | |||
| region: command.region, | |||
| purpose: command.subnetType, | |||
| purpose: command.subnetTypes ? command.subnetTypes[0] : command.subnetType, | |||
There was a problem hiding this comment.
we appear to be using just the first of the list to filter on when before there was only one value that mattered. What's the impact of only filtering by only one subnetType here, and could it effect the resulting vpcId in a way that prevents users from selecting valid resources later (like security groups)?
There was a problem hiding this comment.
Users can only deploy to one VPC at a time (multiple subnets that exist in the same VPC). In this case, all of the subnets will be in the same VPC, so it shouldn't matter which subnet you take, since you're using it to get the vpc id.
| command.vpcId = null; | ||
| result.dirty.vpcId = true; | ||
| } else { | ||
| const subnet = find<ISubnet>(command.backingData.subnets, { | ||
| purpose: command.subnetType, | ||
| purpose: command.subnetTypes ? command.subnetTypes[0] : command.subnetType, |
There was a problem hiding this comment.
same question as above re: filtering.
There was a problem hiding this comment.
Users can only deploy to one VPC at a time (multiple subnets that exist in the same VPC). In this case, all of the subnets will be in the same VPC, so it shouldn't matter which subnet you take, since you're using it to get the vpc id.
|
We don't currently show any of the subnet information (I think those views still need to be built out with complete data). I'll get you some screenshots once I complete testing :) |
piradeepk
force-pushed
the
master
branch
3 times, most recently
from
437ebb5
to
5ae8e73
Compare
The views currently only display the vpc id so it's not an issue.
|
There was a problem hiding this comment.
LGTM! Thanks for the additional screenshots :D
Approved pending approval/merge of related spinnaker/clouddriver#5087 (thx for the 'do not merge' label)
piradeepk
changed the title
piradeepk
added
ready to merge
1d4320a refactor(REST): Prefer REST('/foo/bar') over REST().path('foo', 'bar') 97bfbf6 refactor(api-deprecation): API is deprecated, switch to REST() 39b08e7 refactor(api-deprecation): Prefer API.path('foo', 'bar') over API.path('foo').path('bar') 587db3a refactor(api-deprecation): Migrate from API.one/all/withParams/getList() to path/query/get() 8e2bed6 feat(ecs): support multiple subnet types (#8724)
1d4320a refactor(REST): Prefer REST('/foo/bar') over REST().path('foo', 'bar') 97bfbf6 refactor(api-deprecation): API is deprecated, switch to REST() 39b08e7 refactor(api-deprecation): Prefer API.path('foo', 'bar') over API.path('foo').path('bar') 587db3a refactor(api-deprecation): Migrate from API.one/all/withParams/getList() to path/query/get() 8e2bed6 feat(ecs): support multiple subnet types (#8724)
Add support for multiple subnet types in the ECS Provider.
This change is dependent on spinnaker/clouddriver#5087 being merged in first!
Closes: spinnaker/spinnaker#6097