-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 support pitr restore to sql #6862
Add support pitr restore to sql #6862
Conversation
Okay, cool. What's your plan for dealing with the other required fields? Are you imagining leaving them as required and forcing the user to set them to valid values? And what are you planning for post-create drift for the clone field? |
Leaving them required would indeed not be an ideal solution, I assume it might be possible to use the As for the drift, I'm not quite sure about a good way to handle that. I might have to take a look at how it is handled in other resources/providers or do you have an idea/know of a standardised way of handling this? I'll take a closer look at this tomorrow. |
b6b8812
to
3324b15
Compare
3324b15
to
9c4466a
Compare
We don't have a standard answer to these questions - normally we would probably just leave this in the backlog because we can't think of a good answer to them! If you can, that will be neat. The crucial guiding factors should be a) does not significantly change the experience for non-users of this feature |
What I think we can do to solve both those problems (partially) in this case is the following.
In this case a) the experience is not changed for non-users, apart from some additional lines in the docs explaining the Atleastoneof and b) it should be pretty easy to explain the AtleastOneOf constraint in the docs and mention that the clone configuration is only used during creation, similar to the other settings I believe (according to this comment in the update function "// Update only updates the settings... ). With this approach, there is a slight configuration drift in the clone field (it will only be used during creation, and not be set during import), but this also seems to be the case for some of the other arguments. I've implemented this and it seems to work, unfortunately, I haven't had time to write any tests for it yet. |
Great! I'm comfortable with that as the best possible outcome. One test case for each of the three cases would prove it to me, let me know when you're ready for a review! |
d59abc9
to
fae3432
Compare
I'm sorry for the delay. I've manually tested all three cases by enabling the pitr for the source instance manually. |
9ad1c3b
to
8a48c7c
Compare
8a48c7c
to
edb8380
Compare
Great, it looks like that will be going in today! Let me know when you're ready for review, your approach seems good to me. |
Quick update. I have some trouble with the automatic tests because a backup first has to be created to be able to create a clone but the Manual backup trigger option does not seem to work (aka probably works different then I expect it to work). I have limited time but I hope to finish it tomorrow or Tuesday night. |
Great, thanks for your help! |
Hello, is there any progress to this request? We have a use case where our users only read-only access to Google Cloud Console and would need to be able to execute point-in-time-recovery by supplying the timestamp or backup ID using Terraform. Can this be done using Magic Modules? |
@poj89 It was added in another MR, it was added as: point_in_time_recovery_enabled |
Hello Dorian,
Thanks for replying to my email. We would like to use this feature and have
some questions:
Are the clone and restore_backup_context meant to be used together in the
main.tf configuration file? And is there a way to list out all existing
backup IDs via Terraform?
The optional clone block supports:
-
source_instance_name
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#source_instance_name>
-
(Required) Name of the source instance which will be cloned.
-
point_in_time
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#point_in_time>
-
(Optional) The timestamp of the point in time that should be restored.
A timestamp in RFC3339 UTC "Zulu" format, with nanosecond resolution and
up to nine fractional digits. Examples: "2014-10-02T15:01:23Z" and
"2014-10-02T15:01:23.045123456Z".
-
allocated_ip_range
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#allocated_ip_range>
-
(Optional) The name of the allocated ip range for the private ip CloudSQL
instance. For example: "google-managed-services-default". If set, the
cloned instance ip will be created in the allocated range. The range name
must comply with RFC 1035 <https://tools.ietf.org/html/rfc1035>.
Specifically, the name must be 1-63 characters long and match the regular
expression a-z
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/[-a-z0-9]*[a-z0-9]>
?.
The optional restore_backup_context block supports: NOTE: Restoring from a
backup is an imperative action and not recommended via Terraform. Adding or
modifying this block during resource creation/update will trigger the
restore action after the resource is created/updated.
-
backup_run_id
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#backup_run_id>
-
(Required) The ID of the backup run to restore from.
-
instance_id
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#instance_id>
-
(Optional) The ID of the instance that the backup was taken from. If left
empty, this instance's ID will be used.
-
project
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#project>
-
(Optional) The full project ID of the source instance.
Thanks again.
Regards,
Mario
…On Thu, Mar 3, 2022 at 2:20 AM Dorian de Koning ***@***.***> wrote:
@poj89 <https://github.com/poj89> It was added in another MR, it was
added as: point_in_time_recovery_enabled
<https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance#point_in_time_recovery_enabled>
—
Reply to this email directly, view it on GitHub
<#6862 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJHJ6R3KFMK5HYHE53TOWFLU6BY33ANCNFSM4PJHTTTA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @poj89, I'm afraid I can't really help you out with these questions. I'm also just a user of the provider. I believe we just use the I hope someone else is able to answer your question! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Still work in progress but this add support for creating a sql instance as a clone from another instance at a given point in time. https://cloud.google.com/sql/docs/postgres/admin-api/rest/v1beta4/instances/clone
#6848
I still have to write the tests for this, but I've pushed it to verify the direction I'm taking with this feature is the right one.