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

fix: removing of the deprecated S3 analytics bucket #204

Merged
merged 2 commits into from
Sep 20, 2023
Merged

fix: removing of the deprecated S3 analytics bucket #204

merged 2 commits into from
Sep 20, 2023

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Sep 18, 2023 •
edited
Loading

Description

Removing deprecated AWS S3 bucket and config from the Terraform analytics.
This fixing the applying error because we were moved to a single data lake bucket (prev #159).
The bucket was removed from the data lake itself in the PR #517.

Resolves #203

Tested locally only by validation check terraform validate.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother self-assigned this Sep 18, 2023
@geekbrother geekbrother changed the title removing of the deprecated S3 analytics-data-lake_bucket from terra… fix: removing of the deprecated S3 analytics bucket Sep 18, 2023
dnul
dnul reviewed Sep 18, 2023
View reviewed changes
Copy link

@dnul dnul left a comment

Choose a reason for hiding this comment

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

will this work? looks like on line 7 we're referrencing the resource being removed. I think we should remove everything related to the analytics-data-lake_bucket

chris13524
chris13524 reviewed Sep 18, 2023
View reviewed changes
terraform/analytics/main.tf Outdated
resource "aws_s3_bucket" "analytics-data-lake_bucket" {
bucket = "walletconnect.${var.app_name}.${var.environment}.analytics.data-lake"
}

resource "aws_s3_bucket_acl" "analytics-data-lake_acl" {
bucket = aws_s3_bucket.analytics-data-lake_bucket.id
Copy link
Member

Choose a reason for hiding this comment

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

aws_s3_bucket here refers to the resource you deleted. I believe this will fail to apply

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've missed the elephant in the room 🤦🏻

@geekbrother geekbrother marked this pull request as draft September 19, 2023 06:51
@geekbrother
Copy link
Contributor Author

geekbrother commented Sep 19, 2023 •
edited
Loading

will this work? looks like on line 7 we're referrencing the resource being removed. I think we should remove everything related to the analytics-data-lake_bucket

aws_s3_bucket here refers to the resource you deleted. I believe this will fail to apply

I've missed the elephant in the room 🤦🏻
Seems that the entire terraform/analytics folder and its reference in the main.tf should be removed as we have a unified one S3 bucket for analytics in the data lake right now, and the rest seems not relevant anymore. It seems that first changes to remove it started at #159.

I've updated this PR with the rest.

Also, terraform validate seems happy right now.

@geekbrother geekbrother marked this pull request as ready for review September 19, 2023 09:42
@geekbrother geekbrother merged commit d85ee76 into WalletConnect:main Sep 20, 2023
3 of 5 checks passed
@geekbrother geekbrother deleted the fix/remove_deprecated_analytics_bucket branch September 21, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xav xav xav approved these changes

@chris13524 chris13524 chris13524 approved these changes

@dnul dnul Awaiting requested review from dnul

Assignees

@geekbrother geekbrother

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix: removing of the deprecated analytics S3 bucket from terraform
4 participants
@geekbrother @xav @dnul @chris13524