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.

Sign up for GitHub

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

Jump to bottom

Allow overriding release name #469

Merged
merged 22 commits into from
Sep 6, 2023
Merged

Allow overriding release name #469

merged 22 commits into from
Sep 6, 2023

Conversation

WatcherWhale
Copy link
Contributor

This is a continuation of #416 and fixes #430

@calohmn calohmn added the Hono label Jun 24, 2023
calohmn
calohmn reviewed Jun 24, 2023
View reviewed changes
charts/hono/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/hono/templates/_helpers.tpl Show resolved Hide resolved
@calohmn
Copy link
Contributor

calohmn commented Jun 26, 2023

@WatcherWhale There were changes committed in master (expired Hono Certs getting recreated). Can you rebase?

calohmn
calohmn reviewed Aug 14, 2023
View reviewed changes
charts/hono/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/hono/templates/_helpers.tpl Outdated Show resolved Hide resolved
packages/cloud2edge/Chart.yaml Outdated Show resolved Hide resolved
charts/hono/templates/prometheus/prometheus-config.yaml Outdated Show resolved Hide resolved
charts/hono/values.yaml Outdated Show resolved Hide resolved
charts/hono/templates/kafka/kafka-example-keys.yaml Outdated Show resolved Hide resolved
charts/hono/README.md Outdated Show resolved Hide resolved
@calohmn
Copy link
Contributor

calohmn commented Aug 24, 2023

Looking good! @sophokles73 Anything to add?

sophokles73
sophokles73 reviewed Aug 25, 2023
View reviewed changes
Copy link
Member

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

just some smaller issues
In general, can you also please adapt the copyright headers of the files you changed to reflect the current year?

Thanks for contributing 👍

charts/hono/templates/_helpers.tpl Outdated
*/}}
{{- define "hono.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- $nameOverride := .dot.Values.nameOverride -}}
{{- empty $nameOverride | ternary .dot.Chart.Name $nameOverride | trunc 63 | trimSuffix "-" -}}
Copy link
Member

Choose a reason for hiding this comment

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

this is not totally self-explanatory to me, could you please add a comment describing what name this template actually yields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment and also restructured the code a bit to make it more legible and similar to what is used in hono.fullname

charts/hono/templates/_helpers.tpl Outdated
Comment on lines 39 to 40
{{- $nameOverride := .dot.Values.nameOverride -}}
{{- $name := empty $nameOverride | ternary .dot.Chart.Name $nameOverride -}}
Copy link
Member

Choose a reason for hiding this comment

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

this looks like what is returned by hono.name, right? If so, can we maybe simply invoke that template instead of duplicating the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, as hono.name is truncated to 63 chars and here we still need the full name to check if the release name matches

Copy link
Member

Choose a reason for hiding this comment

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

I see

charts/hono/templates/dispatch-router/dispatch-router-secret.yaml Outdated Show resolved Hide resolved
charts/hono/templates/hono-service-command-router/hono-service-command-router-deployment.yaml Show resolved Hide resolved
charts/hono/example/add_example_data_device_registry.sh Show resolved Hide resolved
charts/hono/values.yaml Outdated Show resolved Hide resolved
@calohmn

This comment was marked as resolved.

Sign in to view
sophokles73
sophokles73 reviewed Aug 29, 2023
View reviewed changes
charts/hono/Chart.yaml Outdated Show resolved Hide resolved
@calohmn calohmn mentioned this pull request Aug 29, 2023
Closed
@calohmn
Copy link
Contributor

calohmn commented Aug 30, 2023

@WatcherWhale Could you rebase and resolve the conflicts? PR #491 has been merged.

vhdirk and others added 11 commits August 30, 2023 09:46
@vhdirk @WatcherWhale
allow overriding release name
299cc8e 
Signed-off-by: Dirk Van Haerenborgh <[email protected]>
@vhdirk @WatcherWhale
fix remarks
eec3a50 
Signed-off-by: Dirk Van Haerenborgh <[email protected]>
@vhdirk @WatcherWhale
let hono.name/hono.fullname accept a template string
02d4845
@vhdirk @WatcherWhale
fix kafka subchart nameoverride
59325fb 
Signed-off-by: Dirk Van Haerenborgh <[email protected]>
@vhdirk @WatcherWhale
fix openshift routes
f748aee 
Signed-off-by: Dirk Van Haerenborgh <[email protected]>
@WatcherWhale
Add prometheus fullname
3b97710 
Signed-off-by: Mathias Maes <[email protected]>
@WatcherWhale
Update release notes
396d590 
Signed-off-by: Mathias Maes <[email protected]>
@WatcherWhale
Remove tpl function
4cf3db5 
Signed-off-by: Mathias Maes <[email protected]>
@WatcherWhale
Add nameOverride for the prometheus config
6a06486 
Signed-off-by: Mathias Maes <[email protected]>
@WatcherWhale
Fix c2e
c65638c 
Signed-off-by: Mathias Maes <[email protected]>
@WatcherWhale
Remove incorrect usage of .dot
1a37f53
@WatcherWhale
Copy link
Contributor Author

@sophokles73 are there any other changes, I still need to apply for the 2 remaining unresolved issues?

@sophokles73
Copy link
Member

are there any other changes, I still need to apply for the 2 remaining unresolved issues?

Well, as my comment, indicates, IMHO it would be good if you added a comment explaining what is actually being computed, as it is not really obvious to me ...

sophokles73
sophokles73 requested changes Sep 5, 2023
View reviewed changes
packages/cloud2edge/Chart.yaml Outdated Show resolved Hide resolved
sophokles73
sophokles73 approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

calohmn
calohmn approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@calohmn calohmn left a comment

Choose a reason for hiding this comment

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

@WatcherWhale Thanks for bringing this to completion!

@calohmn calohmn merged commit f3313cc into eclipse:master Sep 6, 2023
This was referenced Sep 16, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophokles73 sophokles73 sophokles73 approved these changes

@calohmn calohmn calohmn approved these changes

@ctron ctron Awaiting requested review from ctron

Assignees
No one assigned
Labels
Hono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow overriding the hono release name
4 participants
@WatcherWhale @calohmn @sophokles73 @vhdirk