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

FEAT#Allow Enable/Disable Kubernetes Service Links #477

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nkreiger
Copy link

Description of the change

This Change/Proposal, is to allow setting the enableServiceLinks boolean in a kubernetes deployment via the helm charts. This is kind of an obscure configuration ask.

This variable (true by default), will automatically map all the service names and hosts into the environment variables of a kubernetes deployment. However, there is a limit to this, and when that limit is hit, you get the error too many args, whenever a pod restarts or during a new deployment.

Honestly, not sure why the default is true, it should only effect you if you are not using the DNS name when interacting with other services, which is not the case here.

Existing or Associated Issue(s)

Additional Information

Checklist

  • Chart version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
@nkreiger
Copy link
Author

I wasn't sure if I should just keep this forked, or open a PR, if this is something to move forward with, I can do the checklist, let me know.

Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

A few items that need to be added before we can look to integrate this feature:

  • Instead of a dictionary called serviceLinks, what about a boolean called serviceLinksEnabled and use that conditional and value in the deployment?
  • Chart.yaml version bump
  • JSON Schema values update
  • Chart dependencies need to be updated between the charts

Given that this change is spread across multiple charts, separating them out across multiple PR's would be a better approach given the fact that chart testing needs to be able to resolve them effectively

@nkreiger
Copy link
Author

hey @sabre1041 yep, mostly considered this a request, more of the "final" pr. I will spread these out, and use the same strategy here with the dictionary boolean

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.

2 participants