Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[Stable/Telegraf] Allow full use of duplicate input/ouputs in telegraf configuration #4170

Closed
wants to merge 7 commits into from

Conversation

amoskyler
Copy link

What this PR does / why we need it:
This change converts the inputs and outputs fields from an object with keys associated to configs, to an array of objects which contain keys associated to objects.

Before:

config:
  outputs:
     influxdb:
      ...config...

After:

config:
  outputs:
    - influxdb:
       ...config...

Telegraf supports duplicate same type input and output configurations. In the current chart implementation, each input and output is configured with keys in the objects inputs and outputs respectively. YAML overrides the first key in the object however, and therefore the templating process from YAMl to TOML drops configuration valid TOML configurations, which limits . use of possible valid configurations.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amoskyler
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: scottrigby

Assign the PR to them by writing /assign @scottrigby in a comment when ready.

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 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 15, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 15, 2018
@amoskyler
Copy link
Author

CLA signed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 15, 2018
@amoskyler
Copy link
Author

/assign @scottrigby

@amoskyler amoskyler changed the title Allow full use of duplicate input/ouputs in telegraf configuration [Stable/Telegraf] Allow full use of duplicate input/ouputs in telegraf configuration Mar 19, 2018
Copy link
Contributor

@timstoop timstoop left a comment

Choose a reason for hiding this comment

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

I'd love to see some documentation in README as well.

@@ -9,6 +8,8 @@ metadata:
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: "{{ .Release.Name }}"
spec:
updateStrategy:
type: RollingUpdate
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you do not make this settable?

@@ -1,5 +1,5 @@
name: telegraf
version: 0.3.2
version: 0.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

This change adds features, please bump the minor version.

@timstoop
Copy link
Contributor

timstoop commented Apr 1, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 1, 2018
@unguiculus
Copy link
Member

cc @jackzampolin

@amoskyler
Copy link
Author

amoskyler commented Apr 9, 2018

I've added some brief documentation for how to add inputs.
I have also updated the documentation in values.yaml to include details on setting updateStrategy.

Unrelated...Why is this chart marked as deprecated? I don't see any alternative charts. The README mentions telegraf-s and telegraf-ds charts, however I can't find them in the charts repo. They do appear to be defined within this chart however, as there are templates for configmap-ds.yaml as well as configmap-s.yaml

@unguiculus
Copy link
Member

Deprecated charts are not updated. The chart was deprecated in #823.

See https://github.com/kubernetes/charts/blob/master/PROCESSES.md

Please reach out to the original maintainer @jackzampolin in order to see what can be done.

@unguiculus unguiculus closed this Apr 13, 2018
@amoskyler
Copy link
Author

I just went ahead and read that documentation. I didn't read the "deprecated charts are not updated" as an order, but more of implication of the status of deprecation. Why turn down good code?

Do we really need to go through a process where I take over maintenance, make a single PR, and re-deprecate until I'm ready to start the process over again at a later date when I have another PR? (aka, I'm not sure I'm ready to be a maintainer, but I can't contribute to it unless I become the sole maintainer?)

If this project is truly un-maintained, what is the purpose of reaching out to the old maintainer? It would also seem the owner is still listed in the chart, which would seem to be a violation of the deprecation policy.

@amoskyler
Copy link
Author

Also, please note the following issue: #3113

This chart was deprecated in favor of two other charts - which appear to be missing.

@unguiculus
Copy link
Member

Both of these PRs were closed but you can find them upstream in the InfluxDB repo. I thought it would make sense to ask the maintainer if they still intend to add them here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants