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

fix: correctly handle tls ingress #94

Merged
huww98 merged 1 commits from fix-tls-ingress into master 2022-07-28 08:29:33 +00:00
Conversation 14 Commits 1 Files Changed 1 +9 -9
huww98 commented 2020-12-21 04:23:53 +00:00 (Migrated from gitea.com)
Copy Link

This ensures that the the protocol in ROOT_URL is https when configuring TLS in ingress resource. Furthermore, we cannot rely on (index .Values.ingress.tls 0).hosts as it is used for certificates and may contain wildcards.

We also don't need to check .Values.ingress.hosts again, because we already set it to .Values.gitea.config.server.DOMAIN.

Fixes: #307

This ensures that the the protocol in `ROOT_URL` is `https` when configuring TLS in ingress resource. Furthermore, we cannot rely on `(index .Values.ingress.tls 0).hosts` as it is used for certificates and may contain wildcards. We also don't need to check .Values.ingress.hosts again, because we already set it to `.Values.gitea.config.server.DOMAIN`. Fixes: #307
huww98 commented 2020-12-21 04:29:10 +00:00 (Migrated from gitea.com)
Copy Link

Sorry, It does not work, I will make another attempt soon.

Sorry, It does not work, I will make another attempt soon.
huww98 commented 2020-12-21 05:09:26 +00:00 (Migrated from gitea.com)
Copy Link

I've tested it locally this time.

With values.yaml:

ingress:
  enabled: true
  hosts:
  - git.mydomain.com
  tls:
  - hosts:
    - "mydomain.com"
    - "*.mydomain.com"
    secretName: mydomain-cert

it generate:

[server]
APP_DATA_PATH = /data
DOMAIN = git.mydomain.com
HTTP_PORT = 3000
PROTOCOL = http
ROOT_URL = https://git.mydomain.com
SSH_DOMAIN = git.mydomain.com
SSH_LISTEN_PORT = 22
SSH_PORT = 22
I've tested it locally this time. With values.yaml: ```yaml ingress: enabled: true hosts: - git.mydomain.com tls: - hosts: - "mydomain.com" - "*.mydomain.com" secretName: mydomain-cert ``` it generate: ```ini [server] APP_DATA_PATH = /data DOMAIN = git.mydomain.com HTTP_PORT = 3000 PROTOCOL = http ROOT_URL = https://git.mydomain.com SSH_DOMAIN = git.mydomain.com SSH_LISTEN_PORT = 22 SSH_PORT = 22 ```
techknowlogick (Migrated from gitea.com) approved these changes 2020-12-31 05:20:59 +00:00
luhahn commented 2021-01-04 07:54:50 +00:00 (Migrated from gitea.com)
Copy Link

Sorry, for not responding so long, wasn't available during the holidays. I'll take a look at this issue today.

Sorry, for not responding so long, wasn't available during the holidays. I'll take a look at this issue today.
luhahn commented 2021-01-04 14:38:57 +00:00 (Migrated from gitea.com)
Copy Link

I don't really understand why this PR is needed. The protocol will already be set in the config if you enable tls.

It only defaults to http. But you can set it manually.
This is already included and will be integrated in the ROOT_URL

{{- if not .Values.gitea.config.server.PROTOCOL -}}
{{- $_ := set .Values.gitea.config.server "PROTOCOL" "http" -}}
{{- end -}}
...
{{- if .Values.ingress.enabled -}}
    {{- if gt (len .Values.ingress.tls) 0 -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index (index .Values.ingress.tls 0).hosts 0)) -}}
    {{- else -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index .Values.ingress.hosts 0)) -}}
    {{- end -}}
    {{- else -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL .Values.gitea.config.server.DOMAIN) -}}
    {{- end -}}
{{- end -}}
I don't really understand why this PR is needed. The protocol will already be set in the config if you enable tls. It only defaults to http. But you can set it manually. This is already included and will be integrated in the ROOT_URL ``` {{- if not .Values.gitea.config.server.PROTOCOL -}} {{- $_ := set .Values.gitea.config.server "PROTOCOL" "http" -}} {{- end -}} ... {{- if .Values.ingress.enabled -}} {{- if gt (len .Values.ingress.tls) 0 -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index (index .Values.ingress.tls 0).hosts 0)) -}} {{- else -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index .Values.ingress.hosts 0)) -}} {{- end -}} {{- else -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL .Values.gitea.config.server.DOMAIN) -}} {{- end -}} {{- end -}} ```
luhahn (Migrated from gitea.com) reviewed 2021-01-04 14:39:39 +00:00
luhahn (Migrated from gitea.com) left a comment
Copy Link

Please check again if this PR really is needed.

Please check again if this PR really is needed.
huww98 commented 2021-01-04 14:55:25 +00:00 (Migrated from gitea.com)
Copy Link

@luhahn previously, the ROOT_URL is not set correctly. for values.yaml from above comment, it generate ROOT_URL = http://mydomain.com, which is wrong in 2 aspect:

  • The protocol should be https, instead of http, if TLS is enabled.
  • The domain part should be git.mydomain.com (.Values.ingress.hosts[0]), instead of mydomain.com (.Values.ingress.tls[0].hosts[0]), regardless whether TLS is enabled.
@luhahn previously, the `ROOT_URL` is not set correctly. for `values.yaml` from [above comment](https://gitea.com/gitea/helm-chart/issues/94#issuecomment-122715), it generate `ROOT_URL = http://mydomain.com`, which is wrong in 2 aspect: * The protocol should be `https`, instead of `http`, if TLS is enabled. * The domain part should be `git.mydomain.com` (.Values.ingress.hosts[0]), instead of `mydomain.com` (.Values.ingress.tls[0].hosts[0]), regardless whether TLS is enabled.
luhahn (Migrated from gitea.com) reviewed 2021-01-04 17:01:33 +00:00
luhahn (Migrated from gitea.com) reviewed 2021-01-07 15:29:52 +00:00
luhahn (Migrated from gitea.com) left a comment
Copy Link

See my comment, I worry that this could be a breaking change, if someone is using only ingress to define the ROOT_URL

See my comment, I worry that this could be a breaking change, if someone is using only ingress to define the ROOT_URL
luhahn (Migrated from gitea.com) reviewed 2021-03-17 10:02:54 +00:00
huww98 commented 2021-03-17 10:04:11 +00:00 (Migrated from gitea.com)
Copy Link

@luhahn Sorry, I don't recieve the notification. This should not be a breaking change. See my reply above. And I have rebased it on the master.

@luhahn Sorry, I don't recieve the notification. This should not be a breaking change. See my reply above. And I have rebased it on the master.
pat-s commented 2021-11-02 10:56:48 +00:00 (Migrated from gitea.com)
Copy Link

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?
justusbunsi commented 2022-06-26 11:57:17 +00:00 (Migrated from gitea.com)
Copy Link

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?

@pat-s I have the feeling that this PR won't be finished by the original author. It's open for 2 years now and it looks like this PR got out of everyone's sight.

We really need to ensure that the ROOT_URL is set correctly. Especially with the newer versions of Gitea throwing errors to the frontend in case of mismatches.

If no one is against it I will close this PR next weekend (to give some response chances) and propose another PR based on this one.

cc: @luhahn

> @huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this? @pat-s I have the feeling that this PR won't be finished by the original author. It's open for 2 years now and it looks like this PR got out of everyone's sight. We really need to ensure that the ROOT_URL is set correctly. Especially with the newer versions of Gitea throwing errors to the frontend in case of mismatches. If no one is against it I will close this PR next weekend (to give some response chances) and propose another PR based on this one. cc: @luhahn
👍 1
huww98 commented 2022-06-27 04:11:25 +00:00 (Migrated from gitea.com)
Copy Link

Rebased. Hopes the review process can be faster, or there will be only endless conflicts.

Rebased. Hopes the review process can be faster, or there will be only endless conflicts.
🎉 1
justusbunsi commented 2022-07-05 10:08:13 +00:00 (Migrated from gitea.com)
Copy Link

Probably closing #307.

Probably closing #307.
justusbunsi commented 2022-07-05 14:12:22 +00:00 (Migrated from gitea.com)
Copy Link

Hi @huww98. Thanks for your patience and for tackling this issue. I am currently evaluating unit testing for this Helm Chart, so I was curious if your changes would still pass the current main line test cases. You can find the test cases in my fork in a branch including usage instructions. I've included the test results in this comment as collapsed section.

Your changes do fix the wildcard tls + protocol issue for ROOT_URL which can be seen in the last test case (7). It's currently failing because it is designed to success on main line.
But it seems the current changes do not allow overwriting the ROOT_URL anymore (test case 5 of 7). That should still be possible.

@luhahn is right about this PR currently being a breaking change. The default settings rendering changed (test case 2 of 7). As we don't know how many users profit from the current "ingress-is-disabled-but-I-use-its-host-value-anyway" logic. There are two ways of handling this:

  • Remove the check for enabled ingress again.
  • Communicate it being a breaking change via README.md and describe how to mitigate it:
    • If not using built-in ingress but still expose it to the world, users need to define ROOT_URL and DOMAIN.

I'd prefer the first option, TBH.

Unit test results 
<assemblies>
	<assembly name="unittests/config-test.yaml" config-file="unittests/config-test.yaml" test-framework="helm-unittest" run-date="2022-07-05" run-time="16:01:45" time="0.100" total="8" passed="4" failed="4" skipped="0" errors="0">
		<environment>go1.16.9.linux-amd64</environment>
		<errors></errors>
		<collection name="Inline App Configuration Secret" time="0.100" total="8" passed="4" failed="4" skipped="0">
			<test name="renders expected object specs" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass">
				<traits></traits>
			</test>
			<test name="renders default app.ini settings" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -2,3 +2,3 @@ 
			 	   APP_DATA_PATH=/data 
			 	-  DOMAIN=git.example.com 
			 	+  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   ENABLE_PPROF=false 
			 	@@ -6,4 +6,4 @@ 
			 	   PROTOCOL=http 
			 	-  ROOT_URL=http://git.example.com 
			 	-  SSH_DOMAIN=git.example.com 
			 	+  ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local 
			 	+  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   SSH_LISTEN_PORT=22 
]]></stack-trace>
				</failure>
			</test>
			<test name="without no ingress host specification given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.014" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled but no host specs given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `failedTemplate` fail 
			 Error: Invalid rendering: %!s(<nil>) 
]]></stack-trace>
				</failure>
			</test>
			<test name="allows an override for ROOT_URL" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://my.custom.gitea.url 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://my.custom.gitea.url 
			 	  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -2,3 +2,3 @@ 
			 	   APP_DATA_PATH=/data 
			 	-  DOMAIN=git.example.com 
			 	+  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   ENABLE_PPROF=false 
			 	@@ -7,3 +7,3 @@ 
			 	   ROOT_URL=https://my.custom.gitea.url 
			 	-  SSH_DOMAIN=git.example.com 
			 	+  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   SSH_LISTEN_PORT=22 
]]></stack-trace>
				</failure>
			</test>
			<test name="allows an override for DOMAIN" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled and tls configured" type="Inline App Configuration Secret" method="Helm-Validation" time="0.011" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://*.git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -6,3 +6,3 @@ 
			 	   PROTOCOL=http 
			 	-  ROOT_URL=http://*.git.example.com 
			 	+  ROOT_URL=https://git.example.com 
			 	   SSH_DOMAIN=git.example.com 
]]></stack-trace>
				</failure>
			</test>
		</collection>
	</assembly>
</assemblies>

I've also noticed an edge case which isn't considered in main line and not in this PR: Enabled ingress but hosts spec explicitly set to empty array. This leads to a templating error. I've added a commented test case to the test suite covering it. I don't think it's scope of this PR, but important to be aware of.

Hi @huww98. Thanks for your patience and for tackling this issue. I am currently evaluating unit testing for this Helm Chart, so I was curious if your changes would still pass the current main line test cases. You can find the test cases [in my fork in a branch](https://gitea.com/justusbunsi/helm-chart/src/branch/unittesting/unittests) including usage instructions. I've included the test results in this comment as collapsed section. Your changes _do fix_ the wildcard tls + protocol issue for `ROOT_URL` which can be seen in the last test case (7). It's currently failing because it is designed to success on main line. But it seems the current changes do not allow overwriting the `ROOT_URL` anymore (test case 5 of 7). That should still be possible. @luhahn is right about this PR currently being a breaking change. The default settings rendering changed (test case 2 of 7). As we don't know how many users profit from the current "ingress-is-disabled-but-I-use-its-host-value-anyway" logic. There are two ways of handling this: - Remove the check for enabled ingress again. - Communicate it being a breaking change via README.md and describe how to mitigate it: - If not using built-in ingress but still expose it to the world, users need to define `ROOT_URL` and `DOMAIN`. I'd prefer the first option, TBH. <details> <summary>Unit test results</summary> ```xml <assemblies> <assembly name="unittests/config-test.yaml" config-file="unittests/config-test.yaml" test-framework="helm-unittest" run-date="2022-07-05" run-time="16:01:45" time="0.100" total="8" passed="4" failed="4" skipped="0" errors="0"> <environment>go1.16.9.linux-amd64</environment> <errors></errors> <collection name="Inline App Configuration Secret" time="0.100" total="8" passed="4" failed="4" skipped="0"> <test name="renders expected object specs" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass"> <traits></traits> </test> <test name="renders default app.ini settings" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -2,3 +2,3 @@ APP_DATA_PATH=/data - DOMAIN=git.example.com + DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false @@ -6,4 +6,4 @@ PROTOCOL=http - ROOT_URL=http://git.example.com - SSH_DOMAIN=git.example.com + ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local + SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 ]]></stack-trace> </failure> </test> <test name="without no ingress host specification given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.014" result="Pass"> <traits></traits> </test> <test name="with ingress enabled" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Pass"> <traits></traits> </test> <test name="with ingress enabled but no host specs given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `failedTemplate` fail Error: Invalid rendering: %!s(<nil>) ]]></stack-trace> </failure> </test> <test name="allows an override for ROOT_URL" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://my.custom.gitea.url SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://my.custom.gitea.url SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -2,3 +2,3 @@ APP_DATA_PATH=/data - DOMAIN=git.example.com + DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false @@ -7,3 +7,3 @@ ROOT_URL=https://my.custom.gitea.url - SSH_DOMAIN=git.example.com + SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 ]]></stack-trace> </failure> </test> <test name="allows an override for DOMAIN" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass"> <traits></traits> </test> <test name="with ingress enabled and tls configured" type="Inline App Configuration Secret" method="Helm-Validation" time="0.011" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://*.git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -6,3 +6,3 @@ PROTOCOL=http - ROOT_URL=http://*.git.example.com + ROOT_URL=https://git.example.com SSH_DOMAIN=git.example.com ]]></stack-trace> </failure> </test> </collection> </assembly> </assemblies> ``` </details> --- I've also noticed an edge case which isn't considered in main line and not in this PR: Enabled ingress but hosts spec explicitly set to empty array. This leads to a templating error. I've added a [commented test case to the test suite covering it](https://gitea.com/justusbunsi/helm-chart/src/commit/3c846c2be11b2a3259a1dc8565a1dec9c4257e3d/unittests/config-test.yaml#L70). I don't think it's scope of this PR, but important to be aware of.
luhahn commented 2022-07-07 10:51:29 +00:00 (Migrated from gitea.com)
Copy Link

I agree and also would prefer the first option.

I agree and also would prefer the first option.
huww98 commented 2022-07-24 07:28:45 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for your review and detailed explanation. I was mislead by the if .Values.ingress.enabled branch in the original version, and was not aware of this use case.

Now I removed the check for enabled ingress. And verified the default settings rendering is not changed.

Sorry for the delay.

I'm curious if one would set DOMAIN and ROOT_URL to be inconsistent for any reason?

Thanks for your review and detailed explanation. I was mislead by the `if .Values.ingress.enabled` branch in the original version, and was not aware of this use case. Now I removed the check for enabled ingress. And verified the default settings rendering is not changed. Sorry for the delay. I'm curious if one would set DOMAIN and ROOT_URL to be inconsistent for any reason?
luhahn (Migrated from gitea.com) approved these changes 2022-07-26 07:14:13 +00:00
luhahn (Migrated from gitea.com) left a comment
Copy Link

Thanks for hanging on so long, LGTM :)

Thanks for hanging on so long, LGTM :)
justusbunsi (Migrated from gitea.com) approved these changes 2022-07-27 16:46:39 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

? it. Thank you for your patience.

? it. Thank you for your patience.
justusbunsi commented 2022-07-27 16:51:57 +00:00 (Migrated from gitea.com)
Copy Link

I've updated the PR description.

I've updated the PR description.
Sign in to join this conversation.
Reviewers
No Reviewers
techknowlogick
luhahn
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/bug
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#94
No description provided.
Delete Branch "fix-tls-ingress"

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?