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

only return 1 network from api #92

Conversation

bshelton
Copy link
Contributor

@bshelton bshelton commented Oct 20, 2023

This should address #90

@linear
Copy link

linear bot commented Oct 20, 2023

CLO-489 VPC Network Peering Fails Due to Deleted Network Retention

According to this user, our terraform provider fails to provision peerings because it will list a network. I'm not sure but I think they mean the Network data source. We have an integration test for networks with the Terraform provider but don't have anything similar for the Network data source, so it's possible that's broken.

#90

Copy link
Member

@dgivens dgivens left a comment

Choose a reason for hiding this comment

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

The API doesn't prohibit duplicate network names. If someone did have two networks with the same name, the data source would break for them.

I think this may need to change from returning a single network to a slice containing all available matching networks. I'm not sure what the correct way to handle that is in terms of Terraform, because that change in behavior is likely to cause problems for anyone using the data source when they upgrade.

@dgivens
Copy link
Member

dgivens commented Oct 24, 2023

Discussed internally and we're going to create a new data source and deprecate the existing one so we don't break anyone who is already using the current data source by changing the behavior.

@bshelton bshelton force-pushed the brockshelton/clo-489-vpc-network-peering-fails-due-to-deleted-network-retention branch from 1f05ff9 to 4c5f0f6 Compare October 24, 2023 21:46
@bshelton bshelton changed the title only return 1 network from our api only return 1 network from api Dec 4, 2023
Copy link
Contributor

@TimSimpson TimSimpson left a comment

Choose a reason for hiding this comment

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

Let's remove the new data source and the deprecation warning from the current data source which looks great as-is.

We don't need to create a new data source for a list of networks. I'm not a "product" guy but will venture that nobody wants to explicitly name their networks the same such that they could use such a list.

The catalyst for this PR was a user report that the network datasource would consider networks that had been deleted. The second non-explicitly called out problem was that if the data source found more than one network, it would return whichever one was arbitrarily seen first.

This PR fixes both errors by only looking at available networks and returning an error if things are ambiguous. I don't think we need to worry about breaking people who are relying on this behavior, as in most cases we will be helping them to avoid bugs (even if those bugs aren't happening to them yet).

@bshelton bshelton force-pushed the brockshelton/clo-489-vpc-network-peering-fails-due-to-deleted-network-retention branch from c001af7 to 247d131 Compare December 13, 2023 04:12
@bshelton bshelton merged commit 0578bd9 into trunk Jan 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants