allow overriding release name #416

vhdirk · 2022-09-26T14:40:52Z

Fixes #430

Signed-off-by: Dirk Van Haerenborgh [email protected]

calohmn · 2022-09-27T05:33:25Z

@vhdirk The goal of #252 is for the subject alternative names of the example certificates to always match the Hono service names, so that we can enable hostname verification in the clients. With the release name being part of the service name, matching names cannot be guaranteed. Therefore the discussion in #252 lead to the plan to remove the release name from the Hono service names, meaning service names are fixed (see #252 (comment)).

In your PR here, this approach is not taken. Can you explain what the relation to #252 is?

Just looking at the PR changes itself, I see that these would harmonize K8s resource naming with the approach taken in the ditto chart. This would mean that in the cloud2edge chart, service names would then be

c2e-hono-adapter-amqp
c2e-hono-service-auth
c2e-ditto-gateway
c2e-ditto-nginx

and not

c2e-adapter-amqp
c2e-service-auth
c2e-ditto-gateway
c2e-ditto-nginx

any more, making the naming clearer. This will need changes in the cloud2edge chart as well though (e.g. in scripts invoking the services). Therefore we have to think about versioning and compatibility here. (To stay compatible with the cloud2edge chart, we would even have to make this change here a major version jump. I guess we could consider adapting the cloud2edge chart Hono requirements version range though (to ~2.1.0), requiring only a minor version jump here.)

vhdirk · 2022-10-03T09:40:02Z

When asking on gitter if there was an option to override the release name, I was pointed to #252, which is why I referenced it.
I chose not to remove the release name entirely, since we internally also have a service called 'adapter'. The removal of release names from Hono would make it too ambiguous. And we also like the parity with ditto.

I can't imagine removing the prefix altogether would be the only option with regard to hostname verification?

sophokles73 · 2022-10-20T06:27:42Z

When asking on gitter if there was an option to override the release name, I was pointed to #252,

I am sorry, that was my fault. I had simply thought that we could kill two birds with one stone. However, I guess this PR has value on its own, even if it does not address the host name verification problem itself. I agree with @calohmn that we need be transparent about backward compatibility (or the lack thereof) by means of choosing a proper version number increase.

@calohmn apart from the version and corresponding release notes entry, is there anything else that needs to be changed?

I can't imagine removing the prefix altogether would be the only option with regard to hostname verification?

Probably not, I am currently thinking in the direction to (dynamically) create the demo certificates by means of cert-manager, i.e. making cert-manager a pre-requisite for the Hono chart when using demo certs.

sophokles73 · 2022-10-20T15:20:22Z

...mplates/hono-service-device-registry-base/hono-service-device-registry-post-install-job.yaml

@@ -48,6 +48,6 @@ spec:
      volumes:
      - name: post-install-data
        configMap:
-          name: {{ printf "%s-post-install-device-registry-conf" .Release.Name }}
+          name: {{ include "hono.fullname" . }}"-post-install-device-registry-conf"


this produces "hono\"-post-install-device-registry-conf\" which causes the deployment test to fail

I suggest to use

name: {{ printf "%s-post-install-device-registry-conf" ( include "hono.fullname" . ) }}

fixed it for al occurrences of the same thing

sophokles73 · 2022-10-21T07:50:41Z

charts/hono/templates/example-data-grid/statefulset.yaml

@@ -74,5 +74,5 @@ spec:
      volumes:
      - name: conf
        configMap:
-          name: {{ printf "%s-%s-conf" .Release.Name $args.name }}
+          name: {{ printf "%s-%s-conf" (include "hono.name" .) $args.name }}


any particular reason why you are using "hono.name" here instead of "hono.fullname"?
IMHO this is the reason for the deployment tests to still fail ...

I guess the other places in this file where "hono.name" is used also need to be updated ...

Not really, I took inspiration from the ditto helm chart, that is all. I'll change all occurrences

calohmn · 2022-10-21T07:00:50Z

charts/hono/Chart.yaml

@@ -15,7 +15,7 @@ name: hono
 description: |
  Eclipse Hono™ provides remote service interfaces for connecting large numbers of IoT devices to a back end and
  interacting with them in a uniform way regardless of the device communication protocol.
-version: 2.1.5


Regarding cloud2edge compatibility, we have 2 options here:

use version 3.0.0 here. That means the current cloud2edge chart won't reference it (^2.1.0 used there now).

use version 2.2.0 here and first create a new cloud2edge chart version that only changes the Hono dependency requirement from ^2.1.0 to ~2.1.0.

I would tend towards the 2nd option here. Having just the patch-level version range in the cloud2edge chart offers more flexibility regarding Hono chart changes affecting the cloud2edge chart, without having to do a major version jump of the Hono chart each time.
@sophokles73 WDYT?

I also gravitate more towared the second option here.

calohmn · 2022-10-21T07:28:19Z

charts/hono/Chart.yaml

@@ -15,7 +15,7 @@ name: hono
 description: |
  Eclipse Hono™ provides remote service interfaces for connecting large numbers of IoT devices to a back end and
  interacting with them in a uniform way regardless of the device communication protocol.
-version: 2.1.5
+version: 2.1.6


@vhdirk Can you also add an entry in the Release Notes section of the README.md?

done. Changed to v2.2.0 as per #416 (comment)

calohmn · 2022-10-21T07:55:17Z

charts/hono/templates/_helpers.tpl

@@ -102,7 +102,7 @@ The scope passed in is expected to be a dict with keys
 - "component": the value to use for the "app.kubernetes.io/component" label
 */}}
 {{- define "hono.metadata" -}}
-name: {{ printf "%s-%s" .dot.Release.Name .name | quote }}
+name: {{ printf "%s-%s" (include "hono.fullname" .dot ) .name | quote }}


It looks like there are still some more replacements like this needed:

the pod list in NOTES.txt

data-grid server name in hono-data-grid.xml

host entries in qdrouterd.json

HTTP_BASE_URL in add_example_data_device_registry.sh

{{ .Release.Name }}-kafka-example-keys in values.yaml

sophokles73 · 2022-10-28T13:26:41Z

@vhdirk with all the discussion around whether this PR actually helps with fixing the SAN verification problem, I have lost track of what this PR actually tries to achieve. Would you mind creating a dedicated issue describing the lacking functionality and/or problems with the currently implemented naming strategy? We can then better assess, if your PR goes in the right direction or should better take another route to achieving the goal.
Sorry for the inconvenience but I guess the discussion will become more focused this way.

vhdirk · 2022-10-28T13:55:42Z

@sophokles73 no problem, that's perfectly understandable. I've created #430 and linked this PR to it

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

calohmn · 2023-01-03T20:11:20Z

charts/hono/Chart.yaml

@@ -15,7 +15,7 @@ name: hono
 description: |
  Eclipse Hono™ provides remote service interfaces for connecting large numbers of IoT devices to a back end and
  interacting with them in a uniform way regardless of the device communication protocol.
-version: 2.2.0
+version: 2.2.1


There is already a 2.2.1 version now. Can you rebase? As discussed in an earlier comment, this should better be 2.3.0 here.

calohmn · 2023-01-03T20:18:44Z

packages/cloud2edge/Chart.yaml

@@ -11,7 +11,7 @@
 # SPDX-License-Identifier: EPL-2.0
 #
 apiVersion: v1
-version: 0.4.0
+version: 0.4.1


All changes of the cloud2edge chart here should go in a separate PR, to be created once this PR has been merged. This will ensure that the tests for the cloud2edge chart PR will actually run using the Hono chart changes of this PR.

calohmn · 2023-01-03T20:47:02Z

charts/hono/templates/_helpers.tpl

 {{- define "hono.fullname" -}}
-  {{- if .Values.fullnameOverride -}}
-    {{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
+  {{- $fullnameOverride := .dot.Values.fullnameOverride -}}
+  {{- if $fullnameOverride  -}}
+    {{- (tpl $fullnameOverride .dot) | trunc 63 | trimSuffix "-" -}}
  {{- else -}}
-    {{- $name := default .Chart.Name .Values.nameOverride -}}
-    {{- if contains $name .Release.Name -}}
-      {{- .Release.Name | trunc 63 | trimSuffix "-" -}}
+    {{- $nameOverride := .dot.Values.nameOverride -}}
+    {{- $name := empty $nameOverride | ternary .dot.Chart.Name (tpl $nameOverride .dot) -}}


Could you explain the motivation for the change towards requiring a dict with the dot key here (instead of the top-level . scope as before)? This makes this PR quite big (and there are occurrences of include "hono.fullname" . that haven't been replaced yet).
I also wonder whether the tpl function really needs to be used here.

Same applies also to the hono.name, hono.chart and hono.std.labels named templates.

sophokles73 · 2023-08-25T05:46:42Z

@vhdirk can we close this PR?

calohmn · 2023-09-16T08:13:15Z

Closing this PR as this feature was merged via #469.

vhdirk requested review from calohmn and ctron as code owners September 26, 2022 14:40

vhdirk force-pushed the releaseName branch 3 times, most recently from 8945aa8 to ba7c15b Compare September 26, 2022 18:56

calohmn added the Hono label Sep 27, 2022

sophokles73 requested changes Oct 20, 2022

View reviewed changes

vhdirk force-pushed the releaseName branch 3 times, most recently from b36216b to d705224 Compare October 21, 2022 01:24

sophokles73 requested changes Oct 21, 2022

View reviewed changes

calohmn reviewed Oct 21, 2022

View reviewed changes

calohmn mentioned this pull request Nov 3, 2022

Allow overriding the hono release name #430

Closed

vhdirk force-pushed the releaseName branch from a20e6ee to 08b8659 Compare November 15, 2022 12:09

vhdirk added 3 commits December 12, 2022 15:23

allow overriding release name

fb9d629

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

fix remarks

e125904

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

let hono.name/hono.fullname accept a template string

0df4918

vhdirk force-pushed the releaseName branch 3 times, most recently from 3380233 to ef29280 Compare December 12, 2022 14:33

fix kafka subchart nameoverride

ffe258f

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

vhdirk force-pushed the releaseName branch 3 times, most recently from e3560df to a535abb Compare December 12, 2022 15:04

fix openshift routes

b70c4fc

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

vhdirk force-pushed the releaseName branch 3 times, most recently from 6a0902d to 5cf3ad4 Compare December 12, 2022 15:54

fix cloud2edge

8d5c47e

Signed-off-by: Dirk Van Haerenborgh <[email protected]>

vhdirk force-pushed the releaseName branch from 5cf3ad4 to 8d5c47e Compare December 12, 2022 16:04

fix cloud2edge

f12de10

calohmn reviewed Jan 3, 2023

View reviewed changes

WatcherWhale mentioned this pull request May 15, 2023

Allow overriding release name #469

Merged

calohmn closed this Sep 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow overriding release name #416

allow overriding release name #416

vhdirk commented Sep 26, 2022 •

edited

Loading

calohmn commented Sep 27, 2022

vhdirk commented Oct 3, 2022

sophokles73 commented Oct 20, 2022

sophokles73 Oct 20, 2022

vhdirk Oct 20, 2022

sophokles73 Oct 21, 2022

vhdirk Oct 21, 2022

vhdirk Oct 21, 2022

calohmn Oct 21, 2022

vhdirk Oct 21, 2022

calohmn Oct 21, 2022

vhdirk Oct 21, 2022

calohmn Oct 21, 2022

vhdirk Oct 21, 2022

sophokles73 commented Oct 28, 2022

vhdirk commented Oct 28, 2022

calohmn Jan 3, 2023

calohmn Jan 3, 2023

calohmn Jan 3, 2023

sophokles73 commented Aug 25, 2023

calohmn commented Sep 16, 2023

allow overriding release name #416

allow overriding release name #416

Conversation

vhdirk commented Sep 26, 2022 • edited Loading

calohmn commented Sep 27, 2022

vhdirk commented Oct 3, 2022

sophokles73 commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Oct 28, 2022

vhdirk commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Aug 25, 2023

calohmn commented Sep 16, 2023

vhdirk commented Sep 26, 2022 •

edited

Loading