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
Integrated terraform outputs in frontend code-gen #75
Conversation
zthomas
commented
Nov 11, 2019
- updated frontend codebase to work with Cognito
- getting terraform outputs to be plugged back into our config (temporary solution)
- generating react environment variables with Cognito keys
log.Println(aurora.Cyan(emoji.Sprintf("Generating Terraform"))) | ||
terraform.Generate(t, cfg, &wg, pathPrefix) | ||
if cfg.Infrastructure.AWS.Cognito.Deploy { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary solution. We'll need to figure out a system to the scenarios where one part of the code generation requires some output from another step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah, that's gonna get complicated..
templates/terraform/main.tf
Outdated
provider "aws" { | ||
region = "${var.region}" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know where we want to move these configs. I'm just putting everything into the top level main.tf
for now
templates/terraform/variables.tf
Outdated
description = "The AWS region" | ||
} | ||
|
||
# {{ if .Config.Infrastructure.AWS.Terraform.RemoteState }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this portion might be out of date after the last PR merge and we should be able to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed something about the refactor I did.. Let's chat?
} | ||
outputValues := terraform.ExecuteWithOuput(cfg, pathPrefix, outputs) | ||
cfg.Frontend.Env.CognitoPoolID = outputValues["cognito_pool_id"] | ||
cfg.Frontend.Env.CognitoClientID = outputValues["cognito_client_id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should write these values back to the commit0.yml
file..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup for sure, I think we definitely should do that
# domain = "*.${var.public_dns_zone}" | ||
# } | ||
|
||
module "cognito-auth" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module wasn't working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, had some issues. It was pretty complex and had a lot of moving parts. It never ran cleanly for me and also added some dependencies like jq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
description = "AWS Cognito pool name" | ||
} | ||
variable "hostname" { | ||
default = "{{ .Config.Frontend.Hostname }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a default, it should be passed in from where the module is being used.
bucket = "${var.bucket_name}" | ||
// Because we want our site to be available on the internet, we set this so | ||
// anyone can read this bucket. | ||
acl = "public-read" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point this should be changed to private, with proper access set up from Cloudfront, we have plenty of examples of that. Not important for now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup true, I just wanted to be a bit easier for demoing right now. We can just use the s3 url to load it.