-
Notifications
You must be signed in to change notification settings - Fork 0
Setup linkry HTTPS certificate for acm.gg #376
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
Conversation
WalkthroughChanges rename Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
terraform/envs/prod/variables.tf (1)
27-31: Prod-specific LinkryCertificateArn for multi-domain support.The new variable mirrors the QA setup with an appropriate prod account ID and certificate ARN. The inline comment explains the motivation for a separate certificate.
Minor: Correct the typo in the comment on line 27: "seperate" → "separate".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
terraform/envs/prod/main.tf(1 hunks)terraform/envs/prod/variables.tf(1 hunks)terraform/envs/qa/main.tf(1 hunks)terraform/envs/qa/variables.tf(1 hunks)terraform/modules/frontend/main.tf(2 hunks)terraform/modules/frontend/variables.tf(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
🔇 Additional comments (7)
terraform/envs/qa/variables.tf (1)
17-20: LGTM: New LinkryCertificateArn variable addition.The new variable follows the existing naming convention and is correctly positioned. The default ARN is in the us-east-1 region, which is appropriate for CloudFront certificates.
terraform/modules/frontend/main.tf (2)
461-461: Update to CloudFront aliases to support multiple domains.Switching from a single-element list to the
LinkryPublicDomainsvariable enables the frontend module to handle multiple domain aliases. The CloudFront resource correctly accepts a set of domains.
478-478: Separate ACM certificate for Linkry domains.Using a dedicated
LinkryCertificateArninstead of reusingCoreCertificateArnallows independent certificate management for Linkry, necessary for supporting additional domains likeacm.ggin production.Verify that the ACM certificates referenced by
LinkryCertificateArnin both environments are valid and include all required domain aliases (particularlyacm.ggin production andgo.aws.qa.acmuiuc.orgin QA).terraform/modules/frontend/variables.tf (2)
31-34: Variable rename and type promotion for multiple Linkry domains.Renaming
LinkryPublicDomaintoLinkryPublicDomainsand changing the type fromstringtoset(string)correctly enables support for multiple domain aliases. The description is updated accordingly.
42-45: New LinkryCertificateArn variable for dedicated certificate management.Introduces a dedicated ACM certificate variable for Linkry, separating it from the Core certificate. This enables independent lifecycle and domain coverage management.
terraform/envs/qa/main.tf (1)
130-132: QA frontend module updated with new LinkryPublicDomains and LinkryCertificateArn bindings.The QA environment correctly wraps the existing single domain in a list to match the module's updated
set(string)type, and adds the dedicated Linkry certificate ARN. This maintains QA's single-domain setup while supporting the module's new multi-domain capability.terraform/envs/prod/main.tf (1)
127-128: Prod environment adds acm.gg as secondary Linkry domain with dedicated certificate.The production frontend module now includes both
go.acm.illinois.eduandacm.ggas CloudFront aliases, enabling the HTTPS setup for the new domain. The dedicatedLinkryCertificateArnsupports independent certificate management for these domains.Verify that DNS resolution for
acm.ggis properly configured (potentially at the DNS registrar or external provider, since Route53 records are not present in this file). Ensure the ACM certificate referenced byLinkryCertificateArnin production covers bothgo.acm.illinois.eduandacm.ggdomains.
Summary by CodeRabbit