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

ALB as internal/internet facing 🥅 #12

Merged
merged 1 commit into from
Aug 13, 2024
Merged

ALB as internal/internet facing 🥅 #12

merged 1 commit into from
Aug 13, 2024

Conversation

alismx
Copy link
Collaborator

@alismx alismx commented Aug 5, 2024

DEVOPS PULL REQUEST

Related Issue

Changes Proposed

  • Allow for internal or public ALB, defaults to internal
  • update docs and formatting

Additional Information

  • This is a feature request for a client implemenation

@alismx alismx changed the title fix string quote to properly setup tfvars file Optionally set a public ALB Aug 5, 2024
@alismx alismx changed the title Optionally set a public ALB Optionally set a public ALB 🥅 Aug 5, 2024
@alismx alismx changed the title Optionally set a public ALB 🥅 ALB as internal/internet facing 🥅 Aug 5, 2024
Copy link
Collaborator

@shanice-skylight shanice-skylight left a comment

Choose a reason for hiding this comment

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

The code looks straight forward gtg on that. Is there an intended terraform output or simply adding the variable? I ran terraform plan in the ecs directory and was prompted for a name of an App Mesh.

@alismx
Copy link
Collaborator Author

alismx commented Aug 8, 2024

The code looks straight forward gtg on that. Is there an intended terraform output or simply adding the variable? I ran terraform plan in the ecs directory and was prompted for a name of an App Mesh.

@shanice-skylight No output.

If you'd like, we can pair tomorrow and we can go over the requirements to run the terraform. A tfvars file is needed.

Copy link
Collaborator

@shanice-skylight shanice-skylight left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@rin-skylight rin-skylight left a comment

Choose a reason for hiding this comment

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

This looks great! I left an optional consideration for your review, but that does not block this PR from going in.

@@ -31,6 +31,7 @@

| Name | Description | Type | Default | Required |
|------|-------------|------|---------|:--------:|
| <a name="input_alb_internal"></a> [alb\_internal](#input\_alb\_internal) | Whether the ALB is public or private | `bool` | `true` | no |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking, for your consideration: the concepts of "internal" and "private" may be disjoint to certain end users or jurisdictions. You may want to consider buffing the description to include something like, "Whether the ALB is public (intended for external access) or private (only intended to be accessed within your AWS private cloud)."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll include these clarifications on the next PR!

@@ -56,6 +56,7 @@ No modules.

| Name | Description | Type | Default | Required |
|------|-------------|------|---------|:--------:|
| <a name="input_alb_internal"></a> [alb\_internal](#input\_alb\_internal) | Flag to determine if the ALB is public | `bool` | `true` | no |
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above comment regarding the concepts of private vs internal.

@@ -1,3 +1,8 @@
variable "alb_internal" {
type = bool
description = "Flag to determine if the ALB is public"
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above comment regarding the concepts of private vs internal.

@alismx alismx merged commit 7a07d38 into main Aug 13, 2024
2 checks passed
@alismx alismx deleted the alis/deploy-1 branch August 13, 2024 15:54
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.

Allow for setting the ALB to internal or public
3 participants