data/aws/vpc: Drop new_az_count and vpc_id locals #2437
Conversation
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
wking
force-pushed
the
aws-drop-new_az_count
branch
from
67a07be to
95f3de5
Compare
The zone-count variables date back to f828666 (modules/vpc: support re-apply of terraform when AZ number changes, 2018-03-12, coreos/tectonic-installer#3092). But with Terraform 0.12, which we've used since 64c44cd (terraform: bump the vendored version to 0.12-rc.1, 2019-05-14, openshift#1739), we have better array handling, and no longer need count variables. Similarly, there's no need for vpc_id, when we can extract that ID from data.aws_vpc.cluster_vpc.
wking
force-pushed
the
aws-drop-new_az_count
branch
from
95f3de5 to
2dbff62
Compare
|
Rebased onto master now that #2432 has landed. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| protocol = "TCP" | ||
| port = 6443 | ||
| vpc_id = local.vpc_id | ||
| vpc_id = data.aws_vpc.cluster_vpc.id |
There was a problem hiding this comment.
using the data in all the clients, ties the clients to how vpc information is fetched.
using resource -> data source makes CREATE, READ, READ api calls which isn't bad but tying our clients to data source to get the vpc id means we can't change those transparently to all the clients.
So i do prefer the local.vpc_id approach.
There was a problem hiding this comment.
using resource -> data source makes CREATE, READ, READ api calls...
We're already doing that, aren't we? And we're already consuming it here and in other places in master.
which isn't bad but tying our clients to data source to get the vpc id means we can't change those transparently to all the clients.
We can change this transparently by adjusting what we feed into the data block (like this). The consumers just look at data.aws_vpc.cluster_vpc and don't have to care about whether this is a new or preexisting VPC.
So I don't see any functional difference between local.vpc_id and data.aws_vpc.cluster_vpc.id besides the latter being slightly slower due to the latency of the fetch we need to run anyway. And I think adding that latency for consumers that need only the VPC ID is a small price to pay for the benefit of reducing the number of moving parts. Am I missing something? Or do you just think that it's worth having local variables to save the latency when the VPC ID is the only thing we care about?
There was a problem hiding this comment.
And I think adding that latency for consumers that need only the VPC ID is a small price to pay for the benefit of reducing the number of moving parts. Am I missing something?
it's not super important and we can reconsider if it does later on.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Builds on #2432; no need to review until that one lands.
The zone-count variables date back to f828666 (coreos/tectonic-installer#3092). But with Terraform 0.12, which we've used since 64c44cd (#1739), we have better array handling, and no longer need count
variables.
Similarly, there's no need for
vpc_id, when we can extract that ID fromdata.aws_vpc.cluster_vpc.