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

[stable/filebeat] set default config empty #12202

Merged

Conversation

kimxogus
Copy link
Contributor

Signed-off-by: Taehyun Kim kgyoo8232@gmail.com

What this PR does / why we need it:

#12199

Which issue this PR fixes

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

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 14, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @kimxogus. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 14, 2019
@kimxogus
Copy link
Contributor Author

/assign @sstarcher

@sstarcher
Copy link
Collaborator

So by default this will do pretty much nothing. Any reason you could not override the values that you want? I don't believe setting the default setting to be useless is a good move and we should consider something else.

@cdenneen
Copy link
Contributor

helm/helm#5184 if working properly means you can override the output.file to null "remove" it but it doesn't seem to be working properly. If this worked properly you most likely wouldn't need to have empty configuration?

@kimxogus
Copy link
Contributor Author

@sstarcher prospector was deprecated since 6.3, and deprecation warning log is printed in this chart.
Setting filebeat.prospectors as null or empty list [] couldn't unset prospectors, so filebeat.inputs can't be used.

@sstarcher
Copy link
Collaborator

That's fine so this configuration should take a different tact. We should not wipe out the default config.

Possibly a new option

overrideConfig: |
  Whatever random config you want

If overrideConfig is set use it and don't use the default at all.

My concern with these changes is you are breaking everyone and the default configuration for this chart is basically useless. Why don't we try and find a way to make both a reality.

@cdenneen
Copy link
Contributor

@kimxogus when the bug for “null” works properly what issue will that not fix?

@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from f2d2248 to a15719a Compare March 28, 2019 03:36
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2019
@kimxogus
Copy link
Contributor Author

@sstarcher overrideConfig seems like a good solution :)
Added overrideConfig as suggested.

@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from a15719a to 8457e7d Compare March 28, 2019 03:38
@helm-bot helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Mar 28, 2019
@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from 777ab4c to 11bc2ef Compare March 28, 2019 03:41
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Mar 28, 2019
@sstarcher
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 28, 2019
@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from 8c57086 to 17b5a7a Compare April 2, 2019 01:44
@kimxogus
Copy link
Contributor Author

kimxogus commented Apr 2, 2019

merged master

@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from 17b5a7a to 1b2fea1 Compare April 6, 2019 07:50
@kimxogus
Copy link
Contributor Author

kimxogus commented Apr 6, 2019

fixed conflicts. @sstarcher can you please review?

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus kimxogus force-pushed the stable/filebeat/empty-default-config branch from 1b2fea1 to f91a843 Compare April 8, 2019 01:40
@@ -23,7 +23,7 @@ spec:
app: {{ template "filebeat.name" . }}
release: {{ .Release.Name }}
annotations:
checksum/secret: {{ toYaml .Values.config | sha256sum }}
checksum/secret: {{ toYaml (default .Values.config .Values.overrideConfig) | sha256sum }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

going to approve, but I believe it makes it easier to read if written
checksum/secret: {{ .Values.overrideConfig | default .Values.config | toYaml | sha256sum }}

@sstarcher
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimxogus, sstarcher

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 899647f into helm:master Apr 9, 2019
@kimxogus kimxogus deleted the stable/filebeat/empty-default-config branch April 9, 2019 00:55
devnulled pushed a commit to devnulled/charts that referenced this pull request Apr 25, 2019
* add overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* checksum for overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* use default function

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
dermorz pushed a commit to dermorz/charts that referenced this pull request Apr 26, 2019
* add overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* checksum for overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* use default function

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
goshlanguage pushed a commit to goshlanguage/charts that referenced this pull request May 17, 2019
* add overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* checksum for overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* use default function

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
eyenx pushed a commit to eyenx/charts that referenced this pull request May 28, 2019
* add overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* checksum for overrideConfig

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>

* use default function

Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants