Skip to content

Conversation

@OhJuhun
Copy link
Contributor

@OhJuhun OhJuhun commented Jun 4, 2025

Hello I have a problem using this chart.
I want to use remote mongodb, but it doesn't support.
Added this commit to enable remote MongoDB support, which was previously unavailable.

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2025

🦋 Changeset detected

Latest commit: 866b8dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
helm-charts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines 210 to 219
remote: true
endpoint: hyperdx-mongodb-svc
port: 27017
database: hyperdx
size: 10Gi
auth:
enabled: false
username: root
password: password
authSource: admin
Copy link
Collaborator

@wrn14897 wrn14897 Jun 4, 2025

Choose a reason for hiding this comment

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

For style consistency, we can add a custom value mongoUri to hyperdx and inject it into MONGO_URI in the config map (https://github.com/hyperdxio/helm-charts/blob/2f160cdbc4f89c1ec28e1ba18a529bd309c28c3e/charts/hdx-oss-v2/templates/configmaps/app-configmap.yaml#L16)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wrn14897 thanks. I fixed it. did you mean this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect! Left a few comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wrn14897 I fixed all you commented. thanks

usageStatsEnabled: true
# Endpoint to send hyperdx logs/traces/metrics to.Defaults to the chart's otel collector endpoint.
otelExporterEndpoint: http://{{ include "hdx-oss.fullname" . }}-otel-collector:{{ .Values.otel.httpPort }}
mongoUri: mongodb://root:root1234@mongo-uri:27017/hyperdx?authSource=admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

mongodb://{{ include "hdx-oss.fullname" . }}-mongodb:{{ .Values.mongodb.port }}/hyperdx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wrn14897 I didn’t know this syntax was also possible in values.yaml. Thanks.

Comment on lines 16 to 20
{{- if .Values.hyperdx.mongoUri }}
MONGO_URI: {{ .Values.hyperdx.mongoUri | quote }}
{{- else }}
MONGO_URI: mongodb://{{ include "hdx-oss.fullname" . }}-mongodb:{{ .Values.mongodb.service.port }}/hyperdxv2
{{- end }}
Copy link
Collaborator

@wrn14897 wrn14897 Jun 5, 2025

Choose a reason for hiding this comment

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

style: we can do

{{ .Values.hyperdx.mongoUri  |  default (printf ...) }}

{{- else }}
MONGO_URI: mongodb://{{ include "hdx-oss.fullname" . }}-mongodb:{{ .Values.mongodb.service.port }}/hyperdxv2
{{- end }}
MONGO_URI: {{ .Values.hyperdx.mongoUri | default (printf ...) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry I think {{ .Values.hyperdx.mongoUri }} should be sufficient

@wrn14897
Copy link
Collaborator

wrn14897 commented Jun 5, 2025

please also resolve the conflict and we should be good to go

usageStatsEnabled: true
# Endpoint to send hyperdx logs/traces/metrics to.Defaults to the chart's otel collector endpoint.
otelExporterEndpoint: http://{{ include "hdx-oss.fullname" . }}-otel-collector:{{ .Values.otel.httpPort }}
mongoUri: mongodb://{{ include "hdx-oss.fullname" . }}-mongodb:{{ .Values.mongodb.port }}/hyperdx
Copy link
Contributor Author

@OhJuhun OhJuhun Jun 5, 2025

Choose a reason for hiding this comment

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

@wrn14897 I think this doesn't work.

Copy link
Contributor Author

@OhJuhun OhJuhun Jun 5, 2025

Choose a reason for hiding this comment

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

@wrn14897
I think the best way to implement this is writing on _helpers.tpl. How do you think?
like this:

{{- define "hdx-oss.mongodb.uri" -}}
{{- if .Values.hyperdx.mongoUri -}}
    {{- printf .Values.hyperdx.mongoUri -}}
{{- else -}}
    {{- $mongodbPort := .Values.mongodb.port -}}
    {{- printf "mongodb://%s-mongodb:%d/hyperdx" (include "hdx-oss.fullname" .) ( $mongodbPort | int ) -}}
{{- end -}}
{{- end -}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wrn14897 sry. i missed write tpl. i fixed ignore this comments

@OhJuhun
Copy link
Contributor Author

OhJuhun commented Jun 5, 2025

@wrn14897 I fixed all of missed things. test this please

Copy link
Collaborator

@wrn14897 wrn14897 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@wrn14897 wrn14897 merged commit cec5983 into ClickHouse:main Jun 5, 2025
2 checks passed
{{- end -}}
{{- end }}
spec:
{{- if not .Values.mongodb.enabled }}

Choose a reason for hiding this comment

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

When we enable mongodb, the initContainer is removed. Is the not here on purpose or accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junzebao sorry. I missed this, and fixed here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants