Skip to content

Conversation

@mikesmarcos
Copy link

Add information for ingress.useTls value on README.md.
Add useTls use-case on values.yaml.

@mikesmarcos mikesmarcos requested a review from a team as a code owner August 26, 2025 21:50
@joshpollara
Copy link
Contributor

@mikesmarcos Thanks for the PR. I'm not super familiar with Helm charts so I ran this change through Claude Code. Here's what it came back with:

 Critical Bug

  There's a template syntax error in charts/terrateam/templates/terrateam-ingress.yaml on line 31:
  - Current: {{- if and .Values.ingress.useTls }}
  - Should be: {{- if .Values.ingress.useTls }}

  The and operator requires at least two arguments, but only one is provided here. This will cause the Helm template to fail during rendering.

 Suggestions for Improvement

  1. Documentation ordering: For consistency, consider placing the ingress.useTls documentation after ingress.className in the README.md (around line 91) to match the
  order in values.yaml.
  2. Comment clarity: The comment in values.yaml could be more descriptive:
    - Current: "Enable the use of TLS Secret configured at tlsSecretName"
    - Suggested: "Enable TLS for the ingress using the secret specified in tlsSecretName"

It's possible these comments aren't valid! Not sure. Thoughts? Has this change been tested?

@mikesmarcos
Copy link
Author

mikesmarcos commented Aug 27, 2025

@joshpollara hello! :)
Thank you for your time checking my PR.
I've just commited some changes to be closer to the improvements that your AI assistant suggested.
Note that the README.md modification is a bit different than the suggested, but in my opinion it makes much more sense as I submited in this change.

Could you please take a couple of minutes to check again? ;)

Replying about the test: yes, I've tested in my internal environment.
You can make a simple test to validate without install the entire release doing this steps:

  1. to reproduce the bug:
  • create a values.yaml with ingress.useTls ommited or commented AND with ingress.tlsSecretName declared:
ingress:
  annotations: {}
  certificate:
    annotations: {}
    apiVersion: ""
    enabled: true
    kind: ""
    labels: {}
    name: ""
    spec: {}
  className: nginx
  enabled: true
  labels: {}
  name: ingress
  tlsSecretName: my-tls-secret
#  useTls: true (ommit or just remove)
  • run the command below and check the output:

helm template terrateamio/terrateam --values your-values-file.yaml

you will see all the outputs generated by helm and will be able to see that the ingress manifest was not generated with "tls:" attributes, like this:

---
# Source: terrateam/templates/terrateam-ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: terrateam-ingress
  namespace: default
  labels:
    helm.sh/chart: terrateam-1.1.0
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: terrateam
    app.kubernetes.io/version: "latest"
    app.kubernetes.io/name: terrateam-server
spec:
  ingressClassName: nginx
  rules:
  - host: "my.terrateam.host.example"
    http:
      paths:
      - pathType: ImplementationSpecific
        backend:
          service:
            name: terrateam-server
            port:
              name: http
   (tls: values missing here)
  1. validating the solution proposed:
  • change your values.yaml with ingress.useTls DECLARED, and with ingress.tlsSecretName declared:
ingress:
  annotations: {}
  certificate:
    annotations: {}
    apiVersion: ""
    enabled: true
    kind: ""
    labels: {}
    name: ""
    spec: {}
  className: nginx
  enabled: true
  labels: {}
  name: ingress
  tlsSecretName: my-tls-secret
  useTls: true
  • run the command below and check the output:

helm template terrateamio/terrateam --values your-values-file.yaml

you will see all the outputs generated by helm and will be able to see that the ingress manifest now generate the "tls:" attributes correctly, like this:

---
# Source: terrateam/templates/terrateam-ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: terrateam-ingress
  namespace: default
  labels:
    helm.sh/chart: terrateam-1.1.0
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: terrateam
    app.kubernetes.io/version: "latest"
    app.kubernetes.io/name: terrateam-server
spec:
  ingressClassName: nginx
  rules:
  - host: "my.terrateam.host.example"
    http:
      paths:
      - pathType: ImplementationSpecific
        backend:
          service:
            name: terrateam-server
            port:
              name: http
  tls:
  - hosts:
    - "my.terrateam.host.example"
    secretName: my-tls-secret              

Thank you again. Get in touch if you need. ;)

@Invincibear
Copy link
Contributor

@mikesmarcos I'm trying to get these instructions to regenerate the docs merged in, the TLDR is that the README.md should be generated from the comments in values.yaml. I think the plan is to eventually have a gha workflow that does this automatically, but you can run this to regenerate them:

docker run --rm --volume "$(pwd):/helm-docs" -u $(id -u) jnorwood/helm-docs:latest

@thiagowfx
Copy link
Contributor

I'm quite familiar with helm charts, this would be a welcome addition.

@Invincibear
Copy link
Contributor

I think we're still waiting for @mikesmarcos to run the generate-docs script I mentioned in this comment, because the README doesn't match the commend in values.yaml

@joshpollara
Copy link
Contributor

I think we're still waiting for @mikesmarcos to run the generate-docs script I mentioned in this comment, because the README doesn't match the commend in values.yaml

@mikesmarcos Can you please run the script that @Invincibear mentioned then I can get this merged in?

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.

5 participants