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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for restore_to_point_in_time #194

Merged
merged 4 commits into from
Oct 13, 2021
Merged

feat: Add support for restore_to_point_in_time #194

merged 4 commits into from
Oct 13, 2021

Conversation

alacroix
Copy link
Contributor

Description

Add support for the restore_to_point_in_time block (official documentation)

Needed to add a condition on the snapshot_identifier which is incompatible with restore_to_point_in_time if defined. (see hashicorp/terraform-provider-aws#16454)

Motivation and Context

Resolves #192.

Breaking Changes

Not a breaking change but upgraded AWS Terraform provider to >= 3.19 to support this feature with all the fixes. (this one especially hashicorp/terraform-provider-aws#16465 for cross-account cloning)

How Has This Been Tested?

Forked this repo and used it on my company projects for tests environments. Successfully clone an production Aurora MySQL cluster on cross-accounts.

@bryantbiggs bryantbiggs self-requested a review March 2, 2021 11:53
@bryantbiggs
Copy link
Member

hi @alacroix , can you update your PR with the changes from master when you get a chance plese

@alacroix
Copy link
Contributor Author

alacroix commented Mar 4, 2021

@bryantbiggs done 👍

bryantbiggs
bryantbiggs reviewed Mar 6, 2021
View reviewed changes
main.tf Outdated
@@ -57,7 +57,7 @@ resource "aws_rds_cluster" "this" {
port = local.port
db_subnet_group_name = local.db_subnet_group_name
vpc_security_group_ids = compact(concat(aws_security_group.this.*.id, var.vpc_security_group_ids))
snapshot_identifier = var.snapshot_identifier
snapshot_identifier = length(keys(var.restore_to_point_in_time)) == 0 ? var.snapshot_identifier : null
Copy link
Member

Choose a reason for hiding this comment

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

because of this issue hashicorp/terraform-provider-aws#16454 - lets forgo the changes on line 60 and leave it as:

Suggested change
snapshot_identifier = length(keys(var.restore_to_point_in_time)) == 0 ? var.snapshot_identifier : null
snapshot_identifier = var.snapshot_identifier

this defaults to null

bryantbiggs
bryantbiggs reviewed Mar 6, 2021
View reviewed changes
main.tf Outdated
for_each = length(keys(var.restore_to_point_in_time)) == 0 ? [] : [var.restore_to_point_in_time]

content {
source_cluster_identifier = lookup(restore_to_point_in_time.value, "source_cluster_identifier", null)
Copy link
Member

Choose a reason for hiding this comment

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

this attribute is required and should be:

Suggested change
source_cluster_identifier = lookup(restore_to_point_in_time.value, "source_cluster_identifier", null)
source_cluster_identifier = restore_to_point_in_time.value.source_cluster_identifier

@alacroix
Copy link
Contributor Author

alacroix commented Mar 9, 2021

@bryantbiggs thanks for the feedbacks! Done and rebased 👍

@bryantbiggs
Copy link
Member

@alacroix do you know if there is a way we could test this? if so, could you create a test project for this under examples/ - something like examples/restore

@rpadovani
Copy link

This is nice! Can I help to move forward with this and merge this?

@antonbabenko
Copy link
Member

@bryantbiggs I am not sure it is easy to make an example for this feature (it requires two runs of terraform apply). Maybe we can add a section to README.md in the root showing a code snippet and describe how a user can make a cluster restored?

@rpadovani If you can, please update the content of README and resolve conflicts in this one.

@rpadovani
Copy link

@rpadovani If you can, please update the content of README and resolve conflicts in this one.

@antonbabenko I am not the owner of the branch, so I cannot edit it. If you want, I can re-create this PR attributing it to the original author and improving the README.

@antonbabenko
Copy link
Member

@rpadovani That's what I regularly do (if fork belongs to a person and not to the organization where I have no write access):

gh pr checkout 194
# do changes...
git commit -a -m "Something here"
git push

Read more:
https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-active-pull-request-locally

@rpadovani rpadovani mentioned this pull request Oct 13, 2021
Closed
1 task
@rpadovani
Copy link

Read more: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-active-pull-request-locally

Thanks @antonbabenko. I provided creating a new PR: #240

@antonbabenko
Copy link
Member

There is no need in #240. I have just ran the commands from above and pushed 7fdf3d1

@antonbabenko antonbabenko merged commit 929f9ee into terraform-aws-modules:master Oct 13, 2021
@antonbabenko
Copy link
Member

v5.3.0 has been just released.

@rpadovani
Copy link

Thanks, @antonbabenko! Yeah, I cannot do that in this repo, thus the new PR.

I'm happy it has been merged :-) Thanks again for your time on this!

DanK-Ops pushed a commit to usertesting/terraform-aws-rds-aurora that referenced this pull request Nov 18, 2021
Merge branch 'terraform-aws-modules-master' into fetch_upstream_20211118
a1bcc7b 
* terraform-aws-modules-master: (32 commits)
  chore: Giving up on releaserc config (for now) (terraform-aws-modules#257)
  chore(release): version 6.1.3 [skip ci]
  fix: Revert small useless change in main.tf (terraform-aws-modules#256)
  chore(release): version 6.1.2 [skip ci]
  fix: Small useless change in main.tf to test semantic-release (last one, I promise) (terraform-aws-modules#255)
  fix: Small useless change in main.tf to test semantic-release (terraform-aws-modules#254)
  chore: Updated .releaserc config and tf file (to trigger release) (terraform-aws-modules#253)
  chore: Updated .releaserc config (terraform-aws-modules#252)
  chore(release): version 6.1.1 [skip ci]
  chore: Added workflow_dispatch to allow manual releases (terraform-aws-modules#251)
  fix: update CI/CD process to enable auto-release workflow (terraform-aws-modules#250)
  Updated CHANGELOG
  feat: Add security group egress rule support, fix documentation links (terraform-aws-modules#249)
  Updated CHANGELOG
  chore: Updated release Makefile
  Updated CHANGELOG
  BREAKING CHANGE: update module to allow for control over individual cluster instances and latest features (terraform-aws-modules#243)
  chore: update CI workflow to use composite actions, update pre-commit versions (terraform-aws-modules#242)
  Updated CHANGELOG
  feat: Add support for restore_to_point_in_time (terraform-aws-modules#194)
  ...
@github-actions
Copy link

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. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bryantbiggs bryantbiggs bryantbiggs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for restore_to_point_in_time (Aurora Cloning)
4 participants
@alacroix @bryantbiggs @rpadovani @antonbabenko