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

PPM: implement service.targetPort as configurable #648

Merged
merged 10 commits into from
Mar 27, 2025

Conversation

kmasiello
Copy link
Contributor

@kmasiello kmasiello commented Mar 21, 2025

service.targetPort was hard coded as port 4242. Modifed this to be a configurable value to support use cases when customer requires routing to alternate port.

Also implemented pod.port and pod.hostAliases

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025

CLA assistant check
All committers have signed the CLA.

@kmasiello kmasiello requested a review from tnederlof March 21, 2025 16:43
@kmasiello kmasiello marked this pull request as ready for review March 25, 2025 19:17
@kmasiello kmasiello requested a review from a team as a code owner March 25, 2025 19:17
@@ -85,6 +85,8 @@ service:
nodePort: false
# -- The Service port. This is the port your service will run under.
port: 80
# -- The port to forward to on the Package Manager pod. Also see pod.port
targetPort: 4242
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides service.targetPort and pod.port, there are a bunch of other 4242 references in the values, like the config.HTTP.Listen = :4242 setting, liveness/startup probe. I assume users would have to find-and-replace all 4242 references manually to fully change the running port?

Is it possible to use a template variable here so you only have to set 4242 once somewhere?

Otherwise this is fine of course, as it's still better than what we had before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @glin !

I created #652 to capture this. I'd like to see us do this across all three products to be consistent.

@kmasiello kmasiello merged commit b7182ef into main Mar 27, 2025
7 checks passed
@kmasiello kmasiello deleted the km/packagemanager/configurable-targetPort branch March 27, 2025 19:40
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.

None yet

5 participants