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.

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

Parameterize resource name to support watching other resources in events plugin #62

Merged
merged 3 commits into from
Jul 5, 2019

Conversation

maimaisie
Copy link
Contributor

Introduce new config param resource_name to take in a resource name to watch, defaulted to events. Use the same config map to store per-resource RV using key in the resource-version-<resource-name> format.

Minor change: rename apiVersion to api_version to follow the convention of other config param naming.

FYI @rvmiller89 @lei-sumo

Copy link
Contributor

@rvmiller89 rvmiller89 left a comment

Choose a reason for hiding this comment

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

Should we add a test case that covers a resource type other than the default events ?

@maimaisie
Copy link
Contributor Author

@rvmiller89 Yes we probably should. @yuting-liu If you are working on tests for watching events, let's add one for another resource type for example pods

@yuting-liu
Copy link
Contributor

Sure. I'd go ahead adding one test case for this.

Copy link
Contributor

@yuting-liu yuting-liu left a comment

Choose a reason for hiding this comment

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

Should we change this log line to:
Starting collection for #{resource_name}.

@maimaisie
Copy link
Contributor Author

maimaisie commented Jul 3, 2019

I didn't update Starting events collection since we are still collecting events even if the resource name is pods or services, it's just pod events and service events in that case. (this is the reason the plugin is also still named events plugin 😂 ) There is another log line that prints the resource name: New thread to watch #{resource_name} blah..

Copy link
Contributor

@yuting-liu yuting-liu left a comment

Choose a reason for hiding this comment

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

LGTM

@maimaisie
Copy link
Contributor Author

@ggarg2906sumo FYI, after this PR is merged you'll be able to watch events for a specific resource type in K8s. We want to make sure to cover that in testing

@maimaisie maimaisie merged commit 7241daa into master Jul 5, 2019
@maimaisie maimaisie deleted the maisie-resource-name branch July 5, 2019 17:38
psaia pushed a commit to psaia/sumologic-kubernetes-collection that referenced this pull request May 25, 2021
* adding back in ad

* Add enable_ad variable to resources

* Missed enable_ad arg in a couple places

* and one more

* Don't hard code the variable value because duh

* Remove auto_gid

* Don't set enable_ad in collectors because it has a default

* Format terraform

* Add enable_ad to root dir

* does this still work?

* tf fmt

Co-authored-by: Malcolm Jones <17786500+mlclmj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants