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

feat: Logging and Snapshot copy resources converted to standalone resource equivalents, MSV of Terraform raised to v1.3 #99

Conversation

magreenbaum
Copy link
Member

Description

Add aws_redshift_logging and aws_redshift_snapshot_copy resources and removed deprecated configuration blocks for aws_redshift_cluster.logging and aws_redshift_cluster.snapshot_copy.

Motivation and Context

hashicorp/terraform-provider-aws#36862
hashicorp/terraform-provider-aws#36810

Breaking Changes

Yes. New logging and snapshot copy resources replace existing deprecated logging and snapshot_copy blocks in aws_redshift_cluster.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

I don't think this should be a breaking change or major release (v6). Let's remove the Upgrade Guide and just keep changes in the module and example to show the right way to configure these resources.

UPGRADE-6.0.md Show resolved Hide resolved

```bash
terraform import 'module.redshift.aws_redshift_logging.this[0]' <cluster-id>
terraform import 'module.redshift.aws_redshift_snapshot_copy.this[0]' <cluster-id>
Copy link
Member Author

Choose a reason for hiding this comment

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

For those who have snapshot_copy previously configured, this will require an import or it will produce an error that snapshot copy is already enabled when it attempts to create the new resource. I don't mind removing this guide but just want to confirm that we don't want to consider this a breaking change.

╷
│ Error: creating Amazon Redshift Snapshot Copy ("ex-complete"): operation error Redshift: EnableSnapshotCopy, https response error StatusCode: 400, RequestID: caf48715-ac18-4a26-9b28-5c8b647cc5e7, SnapshotCopyAlreadyEnabledFault: Snapshot Copy is already enabled on Cluster ex-complete
│ 
│   with module.redshift.aws_redshift_snapshot_copy.this[0],
│   on ../../main.tf line 321, in resource "aws_redshift_snapshot_copy" "this":
│  321: resource "aws_redshift_snapshot_copy" "this" {
│ 
│ operation error Redshift: EnableSnapshotCopy, https response error StatusCode: 400, RequestID: caf48715-ac18-4a26-9b28-5c8b647cc5e7, SnapshotCopyAlreadyEnabledFault: Snapshot Copy is already enabled on Cluster ex-complete
╵

Copy link
Member

Choose a reason for hiding this comment

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

I see. I thought these resources had the same adoptable behavior as S3 bucket resources like "logging," "versioning," etc. Thank you for the explanation.

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

LGTM, WDYT @bryantbiggs

UPGRADE-6.0.md Outdated Show resolved Hide resolved
UPGRADE-6.0.md Outdated Show resolved Hide resolved

```bash
terraform import 'module.redshift.aws_redshift_logging.this[0]' <cluster-id>
terraform import 'module.redshift.aws_redshift_snapshot_copy.this[0]' <cluster-id>
Copy link
Member

Choose a reason for hiding this comment

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

I see. I thought these resources had the same adoptable behavior as S3 bucket resources like "logging," "versioning," etc. Thank you for the explanation.

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

looks good to me but I do believe its a breaking change. if we do classify it as breaking, I would suggest bumping the MSV of Terraform to 1.3 just to keep things moving forward so that one day we can use some of the newer features

@magreenbaum
Copy link
Member Author

Bumped terraform MSV to 1.3. 👍

UPGRADE-6.0.md Outdated Show resolved Hide resolved
@bryantbiggs bryantbiggs changed the title feat: Logging and Snapshot copy resources feat: Logging and Snapshot copy resources converted to standalone resource equivalents, MSV of Terraform raised to v1.3 May 8, 2024
@bryantbiggs bryantbiggs merged commit 2a2fbdc into terraform-aws-modules:master May 8, 2024
7 checks passed
antonbabenko pushed a commit that referenced this pull request May 8, 2024
## [5.5.0](v5.4.0...v5.5.0) (2024-05-08)

### Features

* Logging and Snapshot copy resources converted to standalone resource equivalents, MSV of Terraform raised to `v1.3` ([#99](#99)) ([2a2fbdc](2a2fbdc))
@antonbabenko
Copy link
Member

antonbabenko commented May 8, 2024

This PR is included in version 6.0.0 🎉

@bryantbiggs
Copy link
Member

crap - started editing title in one window and then switched to a different window - should have been feat!: ... for breaking change

image

I've removed the v5.5.0 tag/release and replaced with a new v6.0.0 tag/release

@magreenbaum
Copy link
Member Author

Ah, my bad. I was wondering what feat! was used for and now I know. Thanks!

@magreenbaum magreenbaum deleted the feat/logging_and_snapshot_copy branch May 8, 2024 00:14
Copy link

github-actions bot commented Jun 7, 2024

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 Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants