-
-
Notifications
You must be signed in to change notification settings - Fork 782
Disable filewatch sensor by default #3293
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
Conversation
These errors in a default installation get tedious
```
2017-03-16 02:16:11,449 140717658023568 ERROR (unknown file) [-] Traceback (most recent call last):
2017-03-16 02:16:11,449 140717658023568 ERROR (unknown file) [-] File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2reactor/container/sensor_wrapper.py", line 357, in <module>
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-]
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-] obj.run()
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-] File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2reactor/container/sensor_wrapper.py", line 214, in run
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-]
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-] self._sensor_instance.setup()
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-] File "/opt/stackstorm/packs/linux/sensors/file_watch_sensor.py", line 20, in setup
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-]
2017-03-16 02:16:11,450 140717658023568 ERROR (unknown file) [-] raise ValueError('No file_paths configured to monitor')
```
This change sets the sensor to disabled by default, so people will need to enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 those errors are annoying.
Just a side note:
Curious how it will work in packaging. For example, user changed the file which is part of the st2 core code, ex: enabled: true for the rule. On next package upgrade the file most probably will be overwritten back to its initial state: enabled: false and sensor will be disabled on st2ctl reload.
Good to think how to improve this story.
|
Yeah, that could be a problem. Issue is that we're mixing configuration with code. Our moves to separate pack config helped a lot, but they don't address this particular issue. Maybe we need to break out sensor state to a separate config? |
|
In general I'm fine with this change, but yes, I also see the problem @armab mentioned, but sadly I see no quick or easy solution for it. |
|
Nope, no easy fix right now. I think that most people don't use this sensor, but they do get the error messages. So we're better off defaulting to disabling it. One of the wrinkles here is that it's a pack, but it's distributed as part of core code. Would be better if it was a pack on ST2 Exchange, and we only installed it if required (this is also related to pack install idempotency I guess) |
|
Yeah, we (dzmine) made that decision to install linux pack by default quite a while ago. I do think it's not a bad idea to have it available by default, but yeah, we eventually need to figure out a better solution but this should do for now (we already do a similar thing for rules). Let's just make sure this is documented in changelog / similar so people won't be surprised if they configure a sensor and it doesn't work. |
These errors in a default installation get tedious
This change sets the sensor to disabled by default, so people will need to enable it.