lunny/helm-chart
1
0
Fork 0
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Improving DRY principle support on gitea Ingress host name #498

Merged
Ceddaerrix merged 0 commits from main into main 2023-09-22 15:09:14 +00:00
Conversation 9 Commits - Files Changed -
Ceddaerrix commented 2023-09-01 10:27:22 +00:00 (Migrated from gitea.com)
Copy Link

Description of the change

Introducing tpl function on variables related to hostname in ./templates/gitea/ingress.yaml

Benefits

The change is intending to support the following syntax in a values.yaml such as:

global:
  giteaHostName: "gitea.my-org.com"

ingress:
  enabled: true
  hosts:
    - host: "{{ .Values.global.giteaHostName }}"
      paths:
        - path: /
          pathType: Prefix
  tls: 
      - secretName: gitea-tls
        hosts:
          - "{{ .Values.global.giteaHostName }}"

Possible drawbacks

N/A

Applicable issues

N/A

Additional information

N/A

### Description of the change Introducing `tpl` function on variables related to hostname in `./templates/gitea/ingress.yaml` ### Benefits The change is intending to support the following syntax in a values.yaml such as: ``` global: giteaHostName: "gitea.my-org.com" ingress: enabled: true hosts: - host: "{{ .Values.global.giteaHostName }}" paths: - path: / pathType: Prefix tls: - secretName: gitea-tls hosts: - "{{ .Values.global.giteaHostName }}" ``` ### Possible drawbacks N/A ### Applicable issues N/A ### Additional information N/A
pat-s commented 2023-09-04 07:04:03 +00:00 (Migrated from gitea.com)
Copy Link

Thanks.

I am not familiar using the tpl function in such cases. I've checked some other charts and none were using tpl within the ingress definition.

Can you assure that this is not a breaking change? And maybe link some existing charts which do this?

Thanks. I am not familiar using the `tpl` function in such cases. I've checked some other charts and none were using `tpl` within the ingress definition. Can you assure that this is not a breaking change? And maybe link some existing charts which do this?
Ceddaerrix commented 2023-09-06 09:41:01 +00:00 (Migrated from gitea.com)
Copy Link

Hello,

The change I have done is backward-compatible.

It allows to use placeholders in the values.yaml file (such as the example in the Benefit section) which are filled when the template are being rendered as explained in https://helm.sh/docs/howto/charts_tips_and_tricks/#using-the-tpl-function.

In short, it evaluates the value that was set in the values.yaml and if the variable has such placeholder, it replaces with value corresponding to the path bound to the placeholder, as it does implicitly in any of the template, else it uses the value as-is.
You can use the example provided with helm template command to see what is generated for ./templates/gitea/ingress.yaml with it. Another advantage is for those using the gitea chart into an umbrella chart and having its default values with the example provided, one could easily override the value of "global.giteaHostName" using the --set flag and it would be applied anywhere "global.giteaHostName" is referenced.

Unfortunately, I have not come by a public chart using the tpl function yet.

ced.

Hello, The change I have done is backward-compatible. It allows to use placeholders in the _values.yaml_ file (such as the example in the Benefit section) which are filled when the template are being rendered as explained in https://helm.sh/docs/howto/charts_tips_and_tricks/#using-the-tpl-function. In short, it evaluates the value that was set in the _values.yaml_ and if the variable has such placeholder, it replaces with value corresponding to the path bound to the placeholder, as it does implicitly in any of the template, else it uses the value as-is. You can use the example provided with _helm template_ command to see what is generated for `./templates/gitea/ingress.yaml` with it. Another advantage is for those using the gitea chart into an umbrella chart and having its default values with the example provided, one could easily override the value of "global.giteaHostName" using the `--set` flag and it would be applied anywhere "global.giteaHostName" is referenced. Unfortunately, I have not come by a public chart using the `tpl` function yet. ced.
pat-s commented 2023-09-06 12:40:44 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for the explanation. Sounds promising. If the tpl function is so flexible, we should consider using it in all places in which . | quote is being used currently.

@justusbunsi What are your thoughts on this?

Thanks for the explanation. Sounds promising. If the `tpl` function is so flexible, we should consider using it in all places in which `. | quote` is being used currently. @justusbunsi What are your thoughts on this?
justusbunsi commented 2023-09-06 17:08:13 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for the explanation. Sounds promising. If the tpl function is so flexible, we should consider using it in all places in which . | quote is being used currently.

@justusbunsi What are your thoughts on this?

tpl is a neat way to DRY. 😀 I even submitted a PR to another Helm Chart in the past to further supporting that1.

So I have nothing against it in our Helm Chart. I'd like to have a test ensuring correct behavior. @Ceddaerrix could you add one please? My linked PR in the Jenkins Helm Chart uses the same toolset IIRC. So you could adapt this into your PR.

EDIT: 😀 Found your PR at the Jenkins chart2. Nice.

@pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community.

> Thanks for the explanation. Sounds promising. If the `tpl` function is so flexible, we should consider using it in all places in which `. | quote` is being used currently. > > @justusbunsi What are your thoughts on this? `tpl` is a neat way to DRY. 😀 I even submitted a PR to another Helm Chart in the past to further supporting that[^1]. So I have nothing against it in our Helm Chart. I'd like to have a test ensuring correct behavior. @Ceddaerrix could you add one please? My linked PR in the Jenkins Helm Chart uses the same toolset IIRC. So you could adapt this into your PR. EDIT: 😀 Found your PR at the Jenkins chart[^2]. Nice. @pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community. [^1]: https://github.com/jenkinsci/helm-charts/pull/509 [^2]: https://github.com/jenkinsci/helm-charts/pull/908
pat-s commented 2023-09-07 13:30:31 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community.

I was thinking about consistency and avoiding many small PRs that add the same logic in multiple places. But I am fine with both.

> @pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community. I was thinking about consistency and avoiding many small PRs that add the same logic in multiple places. But I am fine with both.
justusbunsi commented 2023-09-09 12:53:38 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community.

I was thinking about consistency and avoiding many small PRs that add the same logic in multiple places. But I am fine with both.

TBH, I don't see where else it currently makes sense. I went through the templates and the only part where it would make sense for me are labels or annotations. But I am not fully sure about it either.

For this PR here, I'd expect unit tests ensuring correct rendering behavior. Then I am happy to approve. (@Ceddaerrix)

> > @pat-s Not sure if we should invest time to add it everywhere blindly. If there is a need, there will be a PR. Like this one. So I'd leave it open to the community. > > I was thinking about consistency and avoiding many small PRs that add the same logic in multiple places. But I am fine with both. TBH, I don't see where else it currently makes sense. I went through the templates and the only part where it _would_ make sense for me are labels or annotations. But I am not fully sure about it either. For this PR here, I'd expect unit tests ensuring correct rendering behavior. Then I am happy to approve. (@Ceddaerrix)
👍 1
Ceddaerrix commented 2023-09-19 09:20:12 +00:00 (Migrated from gitea.com)
Copy Link

For this PR here, I'd expect unit tests ensuring correct rendering behavior. Then I am happy to approve. (@Ceddaerrix)

Any guide on the creation of unit test for such case?
I am still ramping up about Helm chart in general

> For this PR here, I'd expect unit tests ensuring correct rendering behavior. Then I am happy to approve. (@Ceddaerrix) Any guide on the creation of unit test for such case? I am still ramping up about Helm chart in general
pat-s commented 2023-09-19 10:11:42 +00:00 (Migrated from gitea.com)
Copy Link

See https://github.com/helm-unittest/helm-unittest and the existing tests in this repo.

See https://github.com/helm-unittest/helm-unittest and the existing tests in this repo.
Ceddaerrix commented 2023-09-20 11:50:03 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi , @pat-s
Please let me know if I should consider more cases in the unit tests or if it is enough.

@justusbunsi , @pat-s Please let me know if I should consider more cases in the unit tests or if it is enough.
justusbunsi (Migrated from gitea.com) approved these changes 2023-09-22 15:05:57 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

LGTM. Thanks for adding the test that ensures the correct behavior.

LGTM. Thanks for adding the test that ensures the correct behavior.
Sign in to join this conversation.
Reviewers
No Reviewers
justusbunsi
Labels
Clear labels
has/backport
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/dependency
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
upstream/other
No Label
kind/enhancement
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#498
No description provided.
Delete Branch "main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?