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

Minor fixes around plugin_directories initialization #5536

Merged

Conversation

@paulkatsoulakis
Copy link
Contributor

paulkatsoulakis commented Mar 2, 2019

Summary

This is a small PR attempting to simplify and clarify the plugin_directories initialization process within daemon. No functional changes should be included within this change. Feel free to recommend further adjustments or even reject the PR if it does not comply with any policy.

The changes include:

  1. Removal of a magic number, primarily because magic numbers are evil and we need to know why we treat the first entry differently (using 0 is counter intuitive)

  2. pull the initialization logic of the plugin_directories variable within the pluginsd scope, only expose a method to trigger it, nothing else needed as it seems (removed a couple of useless globals too)

  3. renamed netdata_configured_plugins_dir to netdata_configured_primary_plugins_dir, because we have multiple directories for the plugins and that parameter actually contains our primary (or stock? not sure its clear to me yet what is the conceptual classification of that first dir)

Component Name

netdata/daemon
netdata/collectors/plugins.d

Additional Information
…he pluginsd where it belongs and call a method from the daemon to do so, 2) Magic numbers are evil, give a meaningful definition for that plugin directories retrieval
…lugins_dir. We allow multiple plugin directories and this is actually the primary (or stock?) directory, just make it clear to avoid confusion
@netdatabot

This comment has been minimized.

Copy link
Member

netdatabot commented Mar 2, 2019

This pull request introduces 1 alert when merging b0386ad into eccf0b9 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

Comment posted by LGTM.com

@paulkatsoulakis

This comment has been minimized.

Copy link
Contributor Author

paulkatsoulakis commented Mar 2, 2019

Thanks bot :P

… place if needed by other modules later
@cakrit
cakrit approved these changes Mar 4, 2019
Copy link
Contributor

cakrit left a comment

Code looks the same, compiled, ran and I don't see anything wrong.

@ktsaou
ktsaou approved these changes Mar 4, 2019
@paulkatsoulakis paulkatsoulakis merged commit b812d23 into netdata:master Mar 5, 2019
12 checks passed
12 checks passed
Header rules - netdata No header rules processed
Details
Pages changed - netdata 2 new files uploaded
Details
Redirect rules - netdata No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Mixed content - netdata No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
netlify/netdata/deploy-preview Deploy preview ready!
Details
@paulkatsoulakis paulkatsoulakis deleted the paulkatsoulakis:patch-minor-fix-for-clarity branch Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.