Cisco-Talos / cvdupdate

ClamAV Private Database Mirror Updater Tool
Apache License 2.0
93 stars 35 forks source link

config.json contains state #27

Closed bsanders closed 2 years ago

bsanders commented 3 years ago

I was attempting to add cvdupdate to a config management tool. We want to keep logs and db in a non-default location, so I need to edit those. config.json has some config options, but also appears to be tracking application state?

Usually I try to just create the config file I want, then compare it to what's on the system (if any), and replace it with the desired one as necessary. But if the config file changes periodically, that doesn't work. Would you be willing to split the dbs portion out to something like ~/.cvdupdate/dbs.json? I might be able to put together a PR with that.

micahsnyder commented 3 years ago

Hm you're right that config.json tracks state. This probably wasn't the best design decision. If you're up for putting together PR, I'm definitely open to splitting the config as you suggest. Maybe state.json or something more intuitive. The uuid field may also be considered "application state" because it should be unique for each system.

bsanders commented 3 years ago

Ok, I'll take a deeper look and see if I can make that happen.

bsanders commented 3 years ago

PR opened #28 - I've tested it locally a few different ways, but I would like to see some unit testing as well. Would you have any issue if I added some unit tests with pytest?

Ah, just realized you wanted UUID in the new file as well. I'll add that change.

micahsnyder commented 3 years ago

PR opened #28 - I've tested it locally a few different ways, but I would like to see some unit testing as well. Would you have any issue if I added some unit tests with pytest?

Unit tests would be great 😍! I was in such a hurry when I first wrote cvdupdate that I didn't include any.

Ah, just realized you wanted UUID in the new file as well. I'll add that change.

Thanks!

bsanders commented 3 years ago

Sorry for the lag, I was on vacation and also trying to do something a bit more clever with the test fixture and an overlay filesystem in docker.

I've moved uuid to state.json and added some tests. Please take a look and let me know.

bsanders commented 3 years ago

Oh, and tests require pytest, and can be run with just pytest . in the root directory of the project. I'd recommend doing it in a VM or docker container, as there is code which will delete ~/.cvdupdate/ between tests. I added a fixtures lookup thing so it would be easy to automatically add other fixtures as needed.

bsanders commented 3 years ago

Ping!

micahsnyder commented 3 years ago

I'd recommend doing it in a VM or docker container, as there is code which will delete ~/.cvdupdate/ between tests. I added a fixtures lookup thing so it would be easy to automatically add other fixtures as needed.

I love that you're adding tests <3 But I'm not really a fan of the idea that the tests could modify/delete files from an existing installation. If you could please adjust the tests so they don't impact existing installations that would be ideal.

bsanders commented 3 years ago

Added more commits to address concerns. If the session fixture isn't enough of a safety net, we could also pull the tests into a separate PR for further work.

micahsnyder commented 2 years ago

Merged your PR today. Thanks again!