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

Add wildcard inclusion/exclusion support to container names #2793

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

m4ce
Copy link
Contributor

@m4ce m4ce commented May 12, 2017

This PR solves #2734 .

@m4ce
Copy link
Contributor Author

m4ce commented May 12, 2017

If the changes look okay, I will go ahead implementing the tests.

@danielnelson
Copy link
Contributor

What are the possible states other than running?

@danielnelson danielnelson added this to the 1.4.0 milestone May 12, 2017
@m4ce
Copy link
Contributor Author

m4ce commented May 13, 2017

@danielnelson, I guess the possible states are: paused, restarting, removing, running, dead, created, exited.

I suppose it only makes sense to track containers in running state? I'd see no reason to extend this to containers in states other than running?

@m4ce m4ce changed the title Optionally disable metrics reporting for non-running containers Disable metrics reporting for non-running containers & add wildcard inclusion/exclusion support to container names May 15, 2017
@m4ce
Copy link
Contributor Author

m4ce commented May 15, 2017

@danielnelson - let me know if the changes look good. Once they do, I will take care of the test units.

@m4ce m4ce force-pushed the 2737 branch 4 times, most recently from 0b7b36e to 88151a8 Compare May 17, 2017 12:54
@m4ce m4ce changed the title Disable metrics reporting for non-running containers & add wildcard inclusion/exclusion support to container names Add wildcard inclusion/exclusion support to container names May 17, 2017
## Only collect metrics for these containers, collect all if empty
container_names = []

## Containers to include and exclude. Globs accepted.
## Note that the options are ignored when using the container_names option. An empty array for both will include all containers
Copy link
Contributor

Choose a reason for hiding this comment

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

We should favor the new options, maybe you can just append container_names to the new value? Then document that the old version is deprecated. Include the version it is deprecated in and the replacement, something like: "Deprecated (1.4.0), use container_name_include"

@m4ce m4ce force-pushed the 2737 branch 4 times, most recently from 11cdd08 to 47a1fec Compare May 21, 2017 19:39
@m4ce
Copy link
Contributor Author

m4ce commented May 23, 2017

@danielnelson - I've made the changes. If they look good, I will proceed with the test cases.

@danielnelson
Copy link
Contributor

Looks good

@m4ce m4ce force-pushed the 2737 branch 2 times, most recently from 1ef8081 to 573d7e7 Compare June 8, 2017 19:54
@m4ce
Copy link
Contributor Author

m4ce commented Jun 8, 2017

@danielnelson - I've added the tests. It's ready to merge.

@danielnelson danielnelson merged commit 4b3b16e into influxdata:master Jun 8, 2017
@danielnelson
Copy link
Contributor

Perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants