Skip to content
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.

Sign up for GitHub

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

Jump to bottom

expose the k8s api version and groups as params #565

Merged
merged 23 commits into from
Jun 4, 2020
Merged

expose the k8s api version and groups as params #565

merged 23 commits into from
Jun 4, 2020

Conversation

vsinghal13
Copy link
Contributor

Description

Fill in your description here.

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

samjsong
samjsong approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM, this would allow for the lookup of resources defined with other api versions, thanks for taking care of this!

vsinghal13
vsinghal13 commented Apr 7, 2020
View reviewed changes
deploy/kubernetes/fluentd-sumologic.yaml.tmpl Outdated
core_api_versions [v1]
api_groups [apps/v1 extensions/v1beta1]
core_api_versions "[v1]"
api_groups "[apps/v1 extensions/v1beta1]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samjsong do we need quotes? Also, inside the brackets, the two items are with no quotes and no comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @perk-sumo

Copy link
Contributor

Choose a reason for hiding this comment

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

that's interesting, can you try a non-helm install and see if this works?

sumo-drosiek
sumo-drosiek approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, as you wrote, we need to ensure that kubernetes yaml works correctly

perk-sumo
perk-sumo suggested changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

Please verify the yaml on the actual cluster before merge.

frankreno
frankreno approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

LGTM, agree with @perk-sumo ask.

vsinghal13 and others added 15 commits April 17, 2020 09:20
@vsinghal13
add fluent bit installation error in doc
e44ec60
@vsinghal13
update params syntax
5391bbe
@vsinghal13
Merge branch 'master' into vsinghal-add-api-version-param
3004059
Generate new 'fluentd-sumologic.yaml.tmpl'
417f426
@vsinghal13
Merge branch 'vsinghal-add-api-version-param' of https://github.com/S…
64e3cec 
…umoLogic/sumologic-kubernetes-collection into vsinghal-add-api-version-param
@vsinghal13
Update values.yaml
9f36452
Generate new 'fluentd-sumologic.yaml.tmpl'
852620b
@vsinghal13
Merge branch 'vsinghal-add-api-version-param' of https://github.com/S…
37a333b 
…umoLogic/sumologic-kubernetes-collection into vsinghal-add-api-version-param
@vsinghal13
fix format
97cc916
Generate new 'fluentd-sumologic.yaml.tmpl'
29bc3b2
@vsinghal13
change to list format in templates
d9fe5be
Generate new 'fluentd-sumologic.yaml.tmpl'
8855860
@vsinghal13
Update values.yaml
81cdfad
Generate new 'fluentd-sumologic.yaml.tmpl'
72fade8
@vsinghal13
comma separated list without qoutes
9d62b17
@vsinghal13
Copy link
Contributor Author

Converted to comma-separated list based on : https://docs.fluentd.org/configuration/config-file

array type: the field is parsed as a JSON array. It also supports
shorthand syntax. These are same values.
normal: ["key1", "key2"]
shorthand: key1,key2

@vsinghal13 vsinghal13 requested a review from perk-sumo June 3, 2020 20:42
samjsong
samjsong approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM :) but the Non_Helm_Installation.md doc changes seem out of place, is that a result of some merge from master or something?

@vsinghal13
Copy link
Contributor Author

LGTM :) but the Non_Helm_Installation.md doc changes seem out of place, is that a result of some merge from master or something?

fixed it

@vsinghal13 vsinghal13 added this to the v1.1 milestone Jun 3, 2020
perk-sumo
perk-sumo approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@perk-sumo perk-sumo merged commit 7aedc57 into master Jun 4, 2020
@perk-sumo perk-sumo deleted the vsinghal-add-api-version-param branch June 4, 2020 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankreno frankreno frankreno approved these changes

@samjsong samjsong samjsong approved these changes

@sumo-drosiek sumo-drosiek sumo-drosiek approved these changes

@perk-sumo perk-sumo perk-sumo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vsinghal13 @frankreno @samjsong @sumo-drosiek @perk-sumo