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

[stable/datadog] add nodePort value in datadog template #6024

Merged
merged 7 commits into from Jun 11, 2018

Conversation

Projects
None yet
5 participants
@whywaita
Contributor

whywaita commented Jun 11, 2018

What this PR does / why we need it:
helm user become able to set fixed dogstatsd nodePort.

if using type: NodePort service, I want to use fixed nodePort.
But, NodePort is random when nodePort is blank.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

in datadog service template, add dogstatsd.nodePort template value.
helm user become able to set fixed port.

Special notes for your reviewer:
This PR don't mention traceport. Because, user need to set .Values.datadog.apmEnabled value when user want to enable traceport.

@whywaita whywaita changed the title from add nodePort value in datadog template to [stable/datadog] add nodePort value in datadog template Jun 11, 2018

@whywaita

This comment has been minimized.

Contributor

whywaita commented Jun 11, 2018

please check me 🙏
/assign @irabinovitch
/assign @hkaj
/assign @xvello

@hkaj

Hi @whywaita - thanks for the PR! I left a couple of comments, let me know what you think.

@@ -1,5 +1,5 @@
name: datadog
version: 0.15.0
version: 0.15.1

This comment has been minimized.

@hkaj

hkaj Jun 11, 2018

Collaborator

This is a minor change, not a bugfix. Please bump to 0.16.0 instead.

This comment has been minimized.

@whywaita

whywaita Jun 11, 2018

Contributor

ok! bumped :) 6e30c6c

@@ -16,6 +16,9 @@ spec:
- port: 8125
name: dogstatsdport
protocol: UDP
{{- if .Values.dogstatsd.nodePort }}

This comment has been minimized.

@hkaj

hkaj Jun 11, 2018

Collaborator

Given the condition on this service's creation I think this should be under the deployment section in values.yaml as dogstatsdNodePort, along with a new traceNodePort.

Could you add the option for traceNodePort as well, along with the corresponding options commented out in values.yaml, with a short comment about what it does? The description from this PR would do.

This comment has been minimized.

@whywaita

whywaita Jun 11, 2018

Contributor

I changed section, to under the deployment.
6a62d25

also, I wrote short description in value.yaml. please check comments!
65d5462

@hkaj

This comment has been minimized.

Collaborator

hkaj commented Jun 11, 2018

/ok-to-test

whywaita added some commits Jun 11, 2018

@k8s-ci-robot k8s-ci-robot added size/S and removed size/XS labels Jun 11, 2018

@hkaj

almost there!

@@ -16,9 +16,15 @@ spec:
- port: 8125
name: dogstatsdport
protocol: UDP
{{- if .Values.deplyment.dogstatsdNodePort }}

This comment has been minimized.

@hkaj

hkaj Jun 11, 2018

Collaborator

typo here: deplyment --> deployment

{{- if .Values.datadog.apmEnabled }}
- port: 8126
name: traceport
protocol: TCP
{{- if .Values.deplyment.traceNodePort }}

This comment has been minimized.

@hkaj

hkaj Jun 11, 2018

Collaborator

same here

This comment has been minimized.

@whywaita

whywaita Jun 11, 2018

Contributor

oh, sorry 😇

fixed 💪 ba64855

@@ -62,6 +62,12 @@ deployment:
# Ref: https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/
tolerations: []
# You'll need to set dogstatsd node port.

This comment has been minimized.

@hkaj

hkaj Jun 11, 2018

Collaborator

Could we do If you're using a NodePort-type service and need a fixed port, set this parameter. instead?

This comment has been minimized.

@whywaita

whywaita Jun 11, 2018

Contributor

Thank you to give nice comment!

ddc6240

whywaita added some commits Jun 11, 2018

@hkaj

This comment has been minimized.

Collaborator

hkaj commented Jun 11, 2018

/approve

@hkaj

This comment has been minimized.

Collaborator

hkaj commented Jun 11, 2018

/test all

@whywaita

This comment has been minimized.

Contributor

whywaita commented Jun 11, 2018

@hkaj Thank you approve!

I think, This PR need to set lgtm label. What should I do?

@hkaj

This comment has been minimized.

Collaborator

hkaj commented Jun 11, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 11, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jun 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hkaj, whywaita

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 7fe4cc5 into helm:master Jun 11, 2018

5 checks passed

ci/circleci: lint-charts Your tests passed on CircleCI!
Details
ci/circleci: lint-scripts Your tests passed on CircleCI!
Details
cla/linuxfoundation whywaita authorized
Details
pull-charts-e2e Job succeeded.
Details
tide In merge pool.
Details

javsalgar added a commit to javsalgar/charts that referenced this pull request Jun 12, 2018

[stable/datadog] add nodePort value in datadog template (helm#6024)
* add nodePort value in datadog template

* bump up datadog chart version

* bump up minor change version

* change value structure, add become able to set traceNodePort

* add description for .Values.deployment.{dogstatsdNodePort, traceNodePort}

* fix typo

* modify description

javsalgar added a commit to javsalgar/charts that referenced this pull request Jun 12, 2018

[stable/datadog] add nodePort value in datadog template (helm#6024)
* add nodePort value in datadog template

* bump up datadog chart version

* bump up minor change version

* change value structure, add become able to set traceNodePort

* add description for .Values.deployment.{dogstatsdNodePort, traceNodePort}

* fix typo

* modify description
@whywaita

This comment has been minimized.

Contributor

whywaita commented Jun 15, 2018

Thank you! :)

or1can added a commit to or1can/charts that referenced this pull request Jul 10, 2018

[stable/datadog] add nodePort value in datadog template (helm#6024)
* add nodePort value in datadog template

* bump up datadog chart version

* bump up minor change version

* change value structure, add become able to set traceNodePort

* add description for .Values.deployment.{dogstatsdNodePort, traceNodePort}

* fix typo

* modify description

voron added a commit to arilot/charts that referenced this pull request Sep 5, 2018

[stable/datadog] add nodePort value in datadog template (helm#6024)
* add nodePort value in datadog template

* bump up datadog chart version

* bump up minor change version

* change value structure, add become able to set traceNodePort

* add description for .Values.deployment.{dogstatsdNodePort, traceNodePort}

* fix typo

* modify description

Signed-off-by: voron <av@arilot.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment