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.

Sign up for GitHub

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

Jump to bottom

feat: add appProtocol and name to var.port_mappings #169

Closed
wants to merge 1 commit into from
Closed

feat: add appProtocol and name to var.port_mappings #169

wants to merge 1 commit into from

Conversation

jtribble
Copy link

@jtribble jtribble commented Sep 26, 2023 •
edited

what

  • Adding missing fields to var.port_mappings (appProtocol, name, containerPortRange)

why

references

@jtribble jtribble requested review from a team as code owners September 26, 2023 17:43
@jtribble jtribble marked this pull request as draft September 26, 2023 17:50
@jtribble jtribble force-pushed the feat/port-mapping-name-doc branch 4 times, most recently from 2c03cfa to 18ede8a Compare September 26, 2023 21:23
@jtribble jtribble marked this pull request as ready for review September 26, 2023 21:25
@jtribble jtribble force-pushed the feat/port-mapping-name-doc branch 3 times, most recently from 5f2d2d5 to 5e7b037 Compare September 26, 2023 21:29
jamengual
jamengual reviewed Sep 27, 2023
View reviewed changes
variables.tf Outdated
Comment on lines 103 to 105
appProtocol = optional(string) # http, http2, grpc (defaults to tcp)
containerPort = optional(number)
containerPortRange = optional(string)
hostPort = optional(number)
name = optional(string)
protocol = optional(string) # tcp (default), udp
Copy link

Choose a reason for hiding this comment

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

can you please reorder this and keep the same order that we have before and add the new vars after?

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing 👍

@jamengual
Copy link

/terratest

@max-lobur
Copy link
Contributor

/terratest

@max-lobur max-lobur enabled auto-merge (squash) September 29, 2023 13:03
@max-lobur
Copy link
Contributor

Just merged earlier PR with the same feature #168

@dudymas
Copy link
Contributor

dudymas commented Sep 29, 2023 •
edited

the other PR had the additional requirement of setting the name field, which is used in the ECS Service Connect api

err... I see this PR also adds name now. Regardless, I hope that #168 accomplishes what folks needed. I saw some issues regarding ordering of the port mapping config. If that's still an issue we should use this PR to re-order them.

@jtribble
Copy link
Author

Hey @max-lobur @dudymas! #168 added name and appProtocol to var.port_mappings, which is the main change I needed! In this PR I also added containerPortRange since I noticed it was missing.

Would you like me to refactor this PR to just contain the port range change? We would also want to make containerPort optional since it's optional in the AWS config.

@jtribble jtribble closed this Oct 2, 2023
auto-merge was automatically disabled October 2, 2023 19:54

Pull request was closed

@jtribble jtribble deleted the feat/port-mapping-name-doc branch October 2, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamengual jamengual jamengual left review comments

@max-lobur max-lobur max-lobur approved these changes

@Gowiem Gowiem Awaiting requested review from Gowiem Gowiem is a code owner automatically assigned from cloudposse/contributors

@korenyoni korenyoni Awaiting requested review from korenyoni korenyoni is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jtribble @jamengual @max-lobur @dudymas