-
Notifications
You must be signed in to change notification settings - Fork 630
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
Add Hyperdrive Resource provider #3111
Add Hyperdrive Resource provider #3111
Conversation
c137204
to
3b1fc83
Compare
changelog detected ✅ |
22164af
to
629df11
Compare
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.
Thank you for implementing this! Just one issue, and that's due to incorrect information in our API spec. We'll make sure to fix that.
638b591
to
8618c40
Compare
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.
Looks good. Thank you @carlos-alberto for this contribution! We have more stuff cooking and I'll try to update this implementation when new things become available.
70eb27e
to
55f6b5e
Compare
@OilyLime I've dropped support for max_age and stale_while_revalidate for now as the api doesn't return these values afaik. @jacobbednarz I've ran the acceptance tests locally and they now pass, but just a heads up that they require a real db to connect to, so will fail if the configuration is not updated to point to a real db. |
@carlos-alberto The API does return I get the following when I curl the API for one of my configs.
|
- Drop support for max_age and stale_while_revalidate as the api does not return these values. The docs also state they are not yet used. - Fix the caching schema to allow it to be computed. I have ran the tests, however they require a real db to connect to, so will fial if the configuration is not updated to point to a real db.
a2047e7
to
ae65f57
Compare
@OilyLime we have a hyperdrive config with id However the api returns:
I wonder if we could land this PR and support for the caching settings could be added later? I think the only way we could support them currently is to duplicate the defaults in the terraform provider, which will make it more difficult for them to be changed later. |
That sounds good to me, thanks again for your valuable contribution @carlos-alberto ! |
looks like the tests are throwing an immediate panic due to not handling failure scenarios in the remote API (lacking entitlements, lacking access, etc). we should rectify this so the provider doesn't end up in an unusable state.
fixed in c2ad546 |
thanks for the PR. i've merged this as is and provided some feedback to the internal team around the config validation to make it a little easier to perform integration testing without a real DB present. |
This functionality has been released in v4.27.0 of the Terraform Cloudflare Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Implements #3092
Requires cloudflare/cloudflare-go#1491 to be released and cloudflare/cloudflare-go#1501 to land.
Note: Not sure if the acctest is actually viable, as I believe DB creds need to actually be valid for the api request to work.