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

AWS MageAI template meant for adding MageAI to existing AWS infrastructure #37

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Ladvien
Copy link

@Ladvien Ladvien commented Jul 20, 2023

Summary

I've modified the mage-ai-terraform-templates/aws module to do the following:

  • Use an existing VPC
  • Moved the infrastructure to a private subnet (or subnet of the code consumer's choosing)
  • Removed public access for the postgres control DB
  • Modified the module to work with Terragrunt
  • Provide an example terragrunt.hcl (inputs) file
  • Created a variable for CIDR's allowed to access the MageAI infrastructure

Tests

It works on my AWS? 😬

cc:
@wangxiaoyou1993, here's the PR I mentioned. Feel free to tell me what you'd like to see different.

@wangxiaoyou1993
Copy link
Member

Thank you @Ladvien for the PR! Will review it today.

variable "allowed_cidr_blocks" {
type = list(string)
description = "List of CIDR blocks to allow access to the ECS service"
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: add empty line at the end of the file

Copy link
Author

Choose a reason for hiding this comment

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

Added

@@ -0,0 +1,3 @@
output "mage_ai_lb_dns_name" {
value = "MageAI URL: ${aws_alb.application_load_balancer.dns_name}"
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: same. empty line.

Copy link
Author

Choose a reason for hiding this comment

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

Added.

}


locals {
Copy link
Member

Choose a reason for hiding this comment

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

I saw you removed the use of "env_vars.json" file. Do you have any particular reason for that?

Copy link
Author

Choose a reason for hiding this comment

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

I did. But none I'm attached to. Adding it back.


## Pre-Deployment
Before deployment, you must have the following installed:
* [Terragrunt](https://terragrunt.gruntwork.io/)
Copy link
Member

Choose a reason for hiding this comment

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

It can also be run without Terragrunt, right?

Copy link
Author

Choose a reason for hiding this comment

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

It can now I've added back the env_vars.json.

@wangxiaoyou1993
Copy link
Member

Hi @Ladvien , just want to make sure you didn't miss my comments. Would you like to address the comments so that we can get the PR merged?

@Ladvien
Copy link
Author

Ladvien commented Jul 29, 2023

Hey @wangxiaoyou1993, it’s on my list. But no time yet.

@Ladvien
Copy link
Author

Ladvien commented Aug 23, 2023

Hey @wangxiaoyou1993, sincere apologies for taking a bit of time to respond. Work got busy.

Just let me know what else you'd like done and I'll try not to take another month to get to it. 😅

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