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

Update simple example #13

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Update simple example #13

merged 1 commit into from
Dec 20, 2024

Conversation

bobbyiliev
Copy link
Collaborator

As discussed internally, making some minor updates to the simple example.

@bobbyiliev bobbyiliev requested a review from kay-kim December 20, 2024 18:38
@kay-kim
Copy link

kay-kim commented Dec 20, 2024

Thank you! ❤️ Running through it now.

@@ -36,7 +36,7 @@ module "materialize_infrastructure" {
bucket_force_destroy = true

# Database Configuration
database_password = "your-secure-password"
database_password = var.database_password
db_identifier = "materialize-simple"
Copy link

@kay-kim kay-kim Dec 20, 2024

Choose a reason for hiding this comment

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

Would it be possible to have a variable name_prefix so that for people can specify a unique name for the following?

  • cluster_name
  • environment
  • vpc_name
  • bucket_name
  • service_account_name
  • db_identifier

Otherwise, I think people in the same org trying this out can run into the same error I did because someone else had run the Terraform first:

╷
│ Error: creating KMS Alias (alias/eks/materialize-eks-simple): operation error KMS: CreateAlias, https response error StatusCode: 400, RequestID: 35377d46-4766-4ba1-a10f-0cac9ca0ee99, AlreadyExistsException: An alias with the name arn:aws:kms:us-east-1:400121260767:alias/eks/materialize-eks-simple already exists
│ 
│   with module.materialize_infrastructure.module.eks.module.eks.module.kms.aws_kms_alias.this["cluster"],
│   on .terraform/modules/materialize_infrastructure.eks.eks.kms/main.tf line 452, in resource "aws_kms_alias" "this":
│  452: resource "aws_kms_alias" "this" {
│ 
╵
╷
│ Error: creating EKS Cluster (materialize-eks-simple): operation error EKS: CreateCluster, https response error StatusCode: 400, RequestID: 72c3a228-78ef-4cfe-b219-da7fdad983f1, InvalidParameterException: For EKS Auto Mode, please ensure that all required configs, including computeConfig, kubernetesNetworkConfig, and blockStorage are all either fully enabled or fully disabled.
│ 
│   with module.materialize_infrastructure.module.eks.module.eks.aws_eks_cluster.this[0],
│   on .terraform/modules/materialize_infrastructure.eks.eks/main.tf line 35, in resource "aws_eks_cluster" "this":
│   35: resource "aws_eks_cluster" "this" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, I was also thinking about this, the current implementation is not great as people need to make sure that they've explicitly set the name for all of the resources individually. I will handle that in a follow-up PR!

Copy link

@kay-kim kay-kim left a comment

Choose a reason for hiding this comment

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

lgtm

@bobbyiliev bobbyiliev merged commit 4b4733e into main Dec 20, 2024
2 checks passed
@bobbyiliev bobbyiliev deleted the update-simple-example branch December 20, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants