-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Datasource for AWS and Fastly IP ranges #7984
Conversation
@yawn this is very cool!! We'll definitely pull this in when the docs and tests are good to go. Thanks for your work on this! 👍 💟 |
Agreed that this is awesome. Thanks so much for this! Perhaps we could rename the |
Not a problem (the renaming to One more question: I'm leaning towards making it an error if the selected filters yield no blocks in the case of the AWS IP ranges but I'm sure if that fits the principle of least surprise (in the context of Terraform). Any opinions regarding this? |
@yawn for several data sources we've been making it an error if the filters are over- or under-constrained. In this case, where the result is a list anyway, that seems less obviously correct since an empty list is a valid result. However, I think I agree that returning no blocks at all, and then in turn e.g. creating a security group that has no blocks, is likely an indication of user error rather than a useful result, so I'm leaning towards agreeing about that being an error at least until we discover there's a use-case for it. |
return fmt.Errorf("Error while converting sync token: %s", err) | ||
} | ||
|
||
d.SetId(result.SyncToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, due to some interactions with some core Terraform code that's common to both data and managed resources, it's required to set an id for a data resource but it doesn't really matter what it's set to... so this should be fine as long as result.SyncToken
is guaranteed to never be the empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we perhaps declare some constant that manifests this fact (e.g. DefaultDataSourceId
) and start to use that? Might be useful since even in the current code the way IDs are derived or generated vary wildly.
Ready for review. |
Thanks for this, @yawn. I expect it will be really useful! |
Glad to be of service @apparentlymart! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This will allow the referencing of IP ranges like so:
If you like this contribution, I'll
blocks
tocidr_blocks
The
main.tf
referenced above should already beapply
able.