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

[network] dont combine connection states #3158

Merged
merged 4 commits into from
Feb 7, 2017
Merged

Conversation

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer commented Jan 30, 2017

This supercedes #2856. It fixes the failing tests and puts the changes behind a flag.

@gmmeyer gmmeyer changed the title Greg/split connection types testing something Jan 30, 2017
@gmmeyer gmmeyer force-pushed the greg/split_connection_types branch from 116111d to f8cc837 Compare February 2, 2017 18:13
@gmmeyer gmmeyer changed the title testing something [network] dont combine connection states Feb 2, 2017
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Added a comment.

Also I think it's worth adding tests with the option enabled and with the existing fixtures, it'll really help if we change the check in the future

@@ -13,3 +13,6 @@ instances:
# Optionally completely ignore any network interface
# matching the given regex:
# excluded_interface_re: my-network-interface.*

# Do not combine connection states
Copy link
Member

Choose a reason for hiding this comment

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

Could you add more details on what the option does? And I think it's worth explicitly mentioning what the default value is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good note, thank you.

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Feb 7, 2017

@olivielpeau I can add some tests, however we'll have to add them in integrations core, the changes to the file structure forced me to change the tests for this one pretty substantially :(

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

👍

I'm fine with writing the new tests directly in integrations-core, let's just add a TODO somewhere so that we don't forget about it :)

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Feb 7, 2017

I'll put it in the issues there

@gmmeyer gmmeyer merged commit 335c432 into master Feb 7, 2017
@gmmeyer gmmeyer deleted the greg/split_connection_types branch February 7, 2017 17:52
degemer added a commit that referenced this pull request Feb 7, 2017
* master: (67 commits)
  [network] dont combine connection states (#3158)
  renames function in line with other checks
  [couchbase] Modified service_check_tags in couchbase.py to include user-specified tags. (#3079)
  [docker] fix image tag extraction
  Fix tests, refactor how we collect container and volume states
  test_docker_daemon.py: fix syntax errors
  Beginning work on docker_daemon tests.
  Add 5 opt-in checks to docker_daemon
  add mention of office hours (#3171)
  updates psycopg2 to 2.6.2 (#3170)
  [trace-agent] adding output of 'trace-agent -info' (#3164)
  [riak] Change default value in configuration example to match default value from the code
  move setting parameter to instance level
  riak security support
  [dns_check][ci] Bring back assertions on metrics (#3162)
  [powerdns_recursor] adds support for v4. (#3166)
  [tcp_check] Add custom tags to response_time gauge
  catch can't connect instead of failing on nodata found (#3127)
  [php-fpm] add http_host tag
  [dns_check] Document NXDOMAIN usage in yaml example file
  ...
@gmmeyer gmmeyer added this to the 5.12.0 milestone Mar 8, 2017
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.

2 participants