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

Fix the default name of the pod #179

Open
alex-bezek opened this issue Mar 13, 2023 · 2 comments
Open

Fix the default name of the pod #179

alex-bezek opened this issue Mar 13, 2023 · 2 comments
Assignees
Labels
area/controller Issues dealing with the controller bug Something isn't working priority/low

Comments

@alex-bezek
Copy link
Collaborator

alex-bezek commented Mar 13, 2023

What happened

The default pod name created by our helm chart is overly verbose and confusing. It should be shortned to be more clear and guessable.

What you think should happen instead

The current name is ngrok-ingress-controller-kubernetes-ingress-controller-manager for me because it combines the release name and the default name https://github.com/ngrok/kubernetes-ingress-controller/blob/main/helm/ingress-controller/templates/_helpers.tpl#L28

Instead of using the regular name and the chart release name, we should just use the override if provided.

How to reproduce

install the chart and get the pods

This is related to #87 since the name is important for your ability to install multiple instances

@alex-bezek alex-bezek added bug Something isn't working area/controller Issues dealing with the controller needs-triage Issues that need triage and removed needs-triage Issues that need triage labels Mar 13, 2023
@Abdiramen
Copy link
Contributor

Abdiramen commented May 25, 2023

Current name outcome matrix

|                  | fullnameOverride              | nameOverride                                 | nothing                                |
|------------------|-------------------------------|----------------------------------------------|----------------------------------------|
| fullnameOverride | fullnameOverride + "-manager" | fullnameOverride                             | fullnameOverride                       |
| nameOverride     | fullnameOverride + "-manager" | chart.name + "-" + nameOverride + "-manager" | chart.name + nameOverride + "-manager" |
| nothing          | fullnameOverride + "-manager" | chart.name + nameOverride + "-manager"       | chart.name + "-" + release.name        |

for example:

  • if nameOverride is set to "my-ingress" then ngrok-ingress-controller-my-ingress-manager-58d8dcbbf5-vlxp9 is then name
  • if fullnameOverride is to "my-ingress" then my-ingress-manager-5865d886d5-x582h is the name
  • if nothing is set set then default name of ngrok-ingress-controller-kubernetes-ingress-controller-manxzprm is used

@nijikokun the current naming scheme is undesirable to customers. I think it would be advantages to just drop the release.name from the default name all together. What do you think?

@nijikokun
Copy link
Contributor

nijikokun commented May 30, 2023

  1. It seems like we are suggesting users to set release.name when installing other controllers leverage the --generate-name flag, we could leverage this.
  2. Desired state would be getting the name down to ngrok-ingress-controller-[hash] without the additional kubernetes-ingress-controller part. Even if that means following other ingress controllers suit where they set the chart name to the company name from kubernetes-ingress-controller to ngrok.

This ideally would mean we wouldn't have to change the repository name as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Issues dealing with the controller bug Something isn't working priority/low
Projects
None yet
Development

No branches or pull requests

4 participants