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

Add global.ingressClassName anchor without default value #31

Merged
merged 8 commits into from
May 9, 2024

Conversation

imgios
Copy link
Collaborator

@imgios imgios commented May 7, 2024

  • Please check if the PR fulfills these requirements
  • The branch naming convention follows our guidelines
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

There's a new global.ingressClassName parameter that can be used to change the Ingress Class.

  • What is the current behavior? (You can also link to an open issue here)

The Ingress Class is set all around the values with the default value nginx.

  • What is the new behavior (if this is a feature change)?

New global parameter global.ingressClassName that makes use of YAML anchors to spread it all around the Chart.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

  • Other information:

None

@imgios imgios added the enhancement New feature or request label May 7, 2024
@imgios imgios requested a review from fonzdm May 7, 2024 20:28
@imgios imgios self-assigned this May 7, 2024
@imgios imgios linked an issue May 7, 2024 that may be closed by this pull request
1 task
@fonzdm
Copy link
Owner

fonzdm commented May 8, 2024

Hi @imgios, thanks for your work!
I was testing out your branch and I found out:

  • when a proper string is defined for the ingressClassName, all ingress resources are created correctly
  • when we leave this parameter empty, we get this (for example, in the sonarr section where the anchor is referenced):
sonarr:
  ingress:
    sonarr-ing:
      annotations:
        cert-manager.io/cluster-issuer: letsencrypt-cloudflare
      enabled: true
      expandObjectName: false
      hosts:
      - host: <REDACTED>
        paths:
        - path: /
          pathType: Prefix
      ingressClassName: null

This will cause the ingress objects to be created like this:

kubectl get ing -n servarr
NAME              CLASS    HOSTS         ADDRESS   PORTS     AGE
radarr-ing        <none>   <REDACTED>              80, 443   4s
jellyfin-ing      <none>   <REDACTED>              80, 443   4s
sonarr-ing        <none>   <REDACTED>              80, 443   4s
prowlarr-ing      <none>   <REDACTED>              80, 443   4s
jellyseerr-ing    <none>   <REDACTED>              80, 443   4s
qbittorrent-ing   <none>   <REDACTED>              80, 443   4s
  • same occurs if we use an empty string (like "")

I don't know of any other option.. if we put "-", we get an error about yaml syntax (probably because the dash sign is interpreted as the beginning of a list).

Probably we just need to force the users to write something here 😅

@imgios
Copy link
Collaborator Author

imgios commented May 8, 2024

Probably we just need to force the users to write something here 😅

Thanks for testing this out. I would more like to leave the anchors out instead of forcing the user to write something. What do you think?

This means that we also have to review the storageClass next.

@imgios
Copy link
Collaborator Author

imgios commented May 8, 2024

Let's do one thing. For the moment, I will use nginx as default value, in this way we have one piece already in place (the anchors that let us change the ingressClass everywhere). The best would be having the dependencies able to read parameters from the global section, I will try to discuss with them 😊

@imgios imgios marked this pull request as draft May 8, 2024 12:23
@imgios imgios marked this pull request as ready for review May 8, 2024 15:38
@imgios imgios marked this pull request as draft May 8, 2024 16:11
@imgios imgios marked this pull request as ready for review May 8, 2024 21:09
servarr/values.yaml Outdated Show resolved Hide resolved
imgios and others added 2 commits May 9, 2024 09:12
Tell the end user to not use &ingressClassName "" otherwise the deployment will use a null value
Copy link
Owner

@fonzdm fonzdm left a comment

Choose a reason for hiding this comment

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

LGTM

@fonzdm fonzdm merged commit bae211d into dev May 9, 2024
@fonzdm fonzdm deleted the feat/ingress-class-name-param branch May 9, 2024 07:31
@fonzdm fonzdm mentioned this pull request May 9, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the ingressClassName optional
2 participants