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

Do not put name of release in resource names by default #57

Closed
Tracked by #4109
yuvipanda opened this issue Sep 15, 2023 · 3 comments · Fixed by #119
Closed
Tracked by #4109

Do not put name of release in resource names by default #57

yuvipanda opened this issue Sep 15, 2023 · 3 comments · Fixed by #119
Assignees

Comments

@yuvipanda
Copy link
Member

yuvipanda commented Sep 15, 2023

If this chart is installed on a namespace named 'imagebuildig-demo' with the name 'imagebuilding-demo', then the service object is called 'imagebuilding-demo/imagebuilding-demo-binderhub-service'. This has a couple of negatives:

  1. There is no single well known url that can be used to setup the service in hub config. It would have to look like this:
      services:
        binder:
          url: http://imagebuilding-demo-binderhub-service:8090
    And require copy pasting for every single deployment, and is error prone.
  2. When using kubectl, this really clutters the view and makes it quite difficult for me to see what is going on. I have to intentionally ignore more than half the characters in names before finding useful information. Reminds me of the dark days of https://en.wikipedia.org/wiki/Hungarian_notation.
  3. The benefit here is that multiple deployments can exist in the same namespace. However, this is an edge case, and not something that should be default optimized for.
  4. There's no way to actually get more concise names by twiddling with config. I tried different variants of the following to no effect:
    binderhub-service:
      fullnameOverride: ""
      nameOverride: ""

I know this is the default behavior of the directory that helm generates, but I'd appreciate us adopting what zero-to-jupyterhub does instead. It defaults to human readable names by default, and for the edge case of people trying to run multiple instances of the chart in one namespace, they can actually set override if they need be.

@consideRatio
Copy link
Contributor

Review -> merge of #119 could close this.

@yuvipanda
Copy link
Member Author

yuvipanda commented Jul 5, 2024

@consideRatio I'd recommend asking one of @sgibson91 or @GeorgianaElena for help reviewing this. Let's open a follow-up ticket about updating 2i2c's infrastructure to use the new setup.

Actually, I've changed my mind. In this instance, given this project is still under the 2i2c umbrella, I'd suggest going with https://infrastructure.2i2c.org/contributing/code-review/#prime-directive instead. Which is to ask - what do you feel uncomfortable about with respect to this PR that another individual person can help with? Can you test it to know it works? If so, I'd recommend testing it and then merging it that way. If there's anything missed, I think that would be caught when we have to change our infrastructure to match.

@yuvipanda
Copy link
Member Author

I've opened 2i2c-org/infrastructure#4370 to track bringing the change in once this is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Todo
Status: Needs Shaping / Refinement
Development

Successfully merging a pull request may close this issue.

2 participants