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

Default CHOP namespace to the release namespace #79

Merged
merged 3 commits into from
Aug 5, 2021
Merged

Conversation

fuziontech
Copy link
Member

@fuziontech fuziontech commented Jul 28, 2021

@macobo
Copy link
Contributor

macobo commented Jul 29, 2021

Wouldn't this break some existing installations since clickhouseOperator would move?

A more sane way is changing the default value to be blank and using default .Values.clickhouseOperator.namespace .Release.namespace

In both cases, going strictly by semver this is a breaking change.

@fuziontech
Copy link
Member Author

fuziontech commented Jul 29, 2021

Wouldn't this break some existing installations since clickhouseOperator would move?

Only if they were installing this off label from our instructions would this be a problem. We have them install this in the posthog namespace in all of our instructions. Doing this now vs later would only make this transition cleaner and less painful.

A more sane way is changing the default value to be blank and using default .Values.clickhouseOperator.namespace .Release.namespace

I would say safe over sane to be more accurate. The correct syntax would be {{ .Values.clickhouseOperator.namespace | default .Release.namespace }}
https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-default-function

In both cases, going strictly by semver this is a breaking change.

This would be breaking, but the remedy would be simply to have existing installations add .Values.clickhouseOperator.namespace back to their provided values.yaml file.

@@ -21,8 +21,6 @@ sentryDSN:
clickhouseOperator:
# -- Whether to install clickhouse. If false, `clickhouse.host` must be set
enabled: true
# -- Which namespace to install clickhouse operator to
namespace: "posthog"
Copy link
Contributor

Choose a reason for hiding this comment

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

keep it just as empty, so we have the description. I believe the default function still works then

Copy link
Member Author

Choose a reason for hiding this comment

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

perfect

@tiina303
Copy link
Contributor

lets yes do the defaulting as Karl suggested (with pipes as that's much better way to use functions).

Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

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

I had a feeling the "" didn't work with default, so I tested it, found another typo. Namespace needs to be capitalized after release.

btw simple way to test this:

helm template posthog charts/posthog --set cloud="x" -n "blah" | grep "clickhouse_service_account" -A5

^ and look at what you see for the namespace

charts/posthog/templates/clickhouse_cluster_role.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_config_map.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_config_map.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_config_map.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_config_map.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_config_map.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_deployment.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_service.yaml Outdated Show resolved Hide resolved
charts/posthog/templates/clickhouse_service_account.yaml Outdated Show resolved Hide resolved
charts/posthog/values.yaml Outdated Show resolved Hide resolved
@tiina303 tiina303 added the bump patch Updates chart version by 0.0.1 label Jul 30, 2021
@tiina303 tiina303 added bump minor Updates the chart version by 0.1.0 and removed bump patch Updates chart version by 0.0.1 labels Aug 5, 2021
@tiina303 tiina303 merged commit 166211c into main Aug 5, 2021
@tiina303 tiina303 deleted the CHOP_namespace branch August 5, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Updates the chart version by 0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install fails on non-posthog namespace
3 participants