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

Add Fluent Bit chart. #895

Merged
merged 12 commits into from Jun 2, 2017
Merged

Add Fluent Bit chart. #895

merged 12 commits into from Jun 2, 2017

Conversation

kfox1111
Copy link
Collaborator

@kfox1111 kfox1111 commented Apr 6, 2017

This PR adds a chart for deploying Fluent Bit that
ships Kubernetes logs.

This change adds a chart for deploying Fluent Bit that
ships Kubernetes logs.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 6, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @kfox1111. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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.

name: {{ template "fullname" . }}-config
labels:
app: {{ template "fullname" . }}
namespace: {{ .Values.namespace }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be left out of the yamls and passed to helm install --namespace foo on the CLI.

kind: DaemonSet
metadata:
name: fluent-bit
namespace: {{ .Values.namespace }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be left out of the yamls and passed to helm install --namespace foo on the CLI.

fluent_bit:
repository: fluent/fluent-bit
tag: 0.11.2
pullPolicy: IfNotPresent
Copy link
Contributor

Choose a reason for hiding this comment

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

pullPolicy seems to be unused in this chart.

# enable if started in minikube.
on_minikube: false

namespace: kube-system
Copy link
Contributor

@linki linki Apr 11, 2017

Choose a reason for hiding this comment

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

namespace should be kept out of the yamls.

@linki
Copy link
Contributor

linki commented Apr 11, 2017

Hi @kfox1111, this charts is missing a NOTES.txt. Here's a good example from the Drupal chart.

@@ -0,0 +1,13 @@
name: fluent-bit
version: 0.11.2
description: Fluent Bit
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a decription somewhere along their description on Github:

Fast and Lightweight Log/Data Forwarder for Linux, BSD and OSX

@kfox1111
Copy link
Collaborator Author

@linki really not sure what to put in a NOTES.txt. Its just a log shipping daemonset.

@kfox1111
Copy link
Collaborator Author

Thanks for the review. I've fixed all the raised issues but the NOTES.txt thing. Please let me know how to proceed there.

@linki
Copy link
Contributor

linki commented Apr 12, 2017

Thanks @kfox1111. How about adding a note that deployment was successful and how to find and follow the logs of fluent-bit itself. Something like this maybe.

@kfox1111
Copy link
Collaborator Author

@linki ok. I think this will address the issue. Thanks for the pointer.

@edsiper
Copy link
Collaborator

edsiper commented Apr 12, 2017

@kfox1111 I've just released image tag 0.11.3, it contains a fix for forward mode.

fluent_bit:
repository: fluent/fluent-bit
tag: 0.11.3
pullPolicy: IfNotPresent
Copy link
Contributor

Choose a reason for hiding this comment

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

pullPolicy is not used in daemonset.yaml

@linki linki added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 14, 2017
@luxas
Copy link

luxas commented Apr 30, 2017

ping @kfox1111 any update on this PR? Would be cool to get in ;)

I think that charts should start in the incubation channel, does that sound good to you?

@kfox1111
Copy link
Collaborator Author

kfox1111 commented May 1, 2017

I think the ball is in other folks court to allow it to merge? I'm not sure the process, nor if I need to do anything else to the ps.

@seanknox
Copy link
Contributor

seanknox commented May 4, 2017

@k8s-bot 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 May 4, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 4, 2017

@kfox1111: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins Charts e2e 8211b01 link @k8s-bot e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@viglesiasce
Copy link
Contributor

@kfox1111 can you give a look at why this is failing to install?

W0503 20:24:55.745] Error: release fluent--1465 failed: error validating "": error validating data: expected type array, for field spec.template.spec.containers[0].env, got map

@edsiper
Copy link
Collaborator

edsiper commented May 24, 2017

Please upgrade Fluent Bit to v0.11.6, it comes with proper X-Pack support for Elasticsearch

@kfox1111
Copy link
Collaborator Author

Anything else needing to be done for this?

@edsiper
Copy link
Collaborator

edsiper commented May 31, 2017

Upgrade to v0.11.7 XD

@@ -0,0 +1,13 @@
name: fluent-bit
version: 0.11.7
Copy link
Member

Choose a reason for hiding this comment

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

This should be the appVersion, the chart version should be 0.1.0

Expand the name of the chart.
*/}}
{{- define "name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 -}}
Copy link
Member

Choose a reason for hiding this comment

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

add trimSuffix "-" at the end

- add appVersion, icon and home
- update maintainers to use github ids
@prydonius prydonius added code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed and removed changes needed labels Jun 2, 2017
@prydonius prydonius merged commit 9d3676a into helm:master Jun 2, 2017
@kfox1111 kfox1111 deleted the fluent-bit branch June 2, 2017 15:42
lachie83 added a commit to lachie83/charts that referenced this pull request Jun 2, 2017
* upstream/master:
  Add Fluent Bit chart. (helm#895)
  Existing PVC support for stable/drupal (helm#1084)
  [stable/nginx-ingress] - Adding in support for the publish-service parameter (helm#975)
  [stable/prometheus] Add extraHostPathMounts config (helm#862)
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
* Add Fluent Bit chart.

This change adds a chart for deploying Fluent Bit that
ships Kubernetes logs.

* Fix some misnamed entries.

* Add forwarder Retry_Limit

* Remove namespace. Update description.

* Add Notes.txt

* Bump version to 0.11.3

* Add missing imagePullPolicy

* Fix wrong values datatype for env.

* Bump version to 0.11.6

* Bump fluent-bit version.

* Update Chart.yaml

- add appVersion, icon and home
- update maintainers to use github ids

* add trimSuffix to helpers
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. code reviewed lgtm Indicates that a PR is ready to be merged. size/medium UX reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants