break up the code #236
Replies: 10 comments 28 replies
-
Hi, |
Beta Was this translation helpful? Give feedback.
-
Hi Jakob, I have done some more cleanup but am getting to the limit without breaking down things into smaller methods. Timezone also seem to be used inconsistently and did not work for me. So I removed all the TZ from the docker build and just pass in the correct timezone, that seems to work for me, but there was a lot of code in the files, so I assume it did not work for you previously... For testing I am trying a normal scan, nmap scan, pihole network and pihole dhcp. |
Beta Was this translation helpful? Give feedback.
-
Hi, I'll try answering what I know. Arpscan was the the only scan that was available in the original code. I wanted to make it more extensible by creating a separate nmap scan table. I realised that doing this for every scan is unsustainable so I came up with the plugin system. The Nmap scan could be rewritten into a plugin later on. Cycle - no idea now. Tried to remove it but it's used in way too many sql queries. Was used in the old code to change the frequency of the scan when the execution was based on a cron script. TZ was also previously used for adjusting the cron script so if it can be used without then great. Although ideally existing docker compose set ups should work. It would be great to test a couple of the plugins and the notifications, but I could do that probably over the next weekend. Thanks! |
Beta Was this translation helpful? Give feedback.
-
Alright, your changes are merged (hopefully correctly) into this branch: https://github.com/jokob-sk/Pi.Alert/tree/split-up-work-2023-05-30 There is also a dev image to test: https://registry.hub.docker.com/r/jokobsk/pi.alert_dev
Thanks again for all the work! @Data-Monkey BTW - try pulling the default DB file into your branch, there is still some other DB file present. |
Beta Was this translation helpful? Give feedback.
-
Thanks for merging all my code back into your project. That was a brave move! |
Beta Was this translation helpful? Give feedback.
-
Thank you for spending so much time on this! Where next? I don't have any specific plan. Usually there is a feature request that inspires me to implement some broader functionality. Now I'm mostly thinking about how to enable the community to contribute, such as via the plugins or detailed docs. Creating notification plugin system and/or rewriting the Nmap scan into a plugin could be next logical steps. Open to suggestions though. 😉 |
Beta Was this translation helpful? Give feedback.
-
some good ideas! I like the plugin idea and maybe also dividing the "scanners" into different types of scanners. But I also expect there will be many bugs to iron out with the split up version. I certainly did not manage to run all the code yet. |
Beta Was this translation helpful? Give feedback.
-
Yep, agree on possible improvements. You probably know how it is, I also try to balance personal life lol, so sometimes I have bandwidth for sweeping changes and sometimes for high priority maintenance tasks only. Re bugs, I was running the build for a week and hope I've tested most scenarios, but no bugs no progress and I hope the users will be understanding 👍 |
Beta Was this translation helpful? Give feedback.
-
Hi Jakob, |
Beta Was this translation helpful? Give feedback.
-
Thanks @Data-Monkey! I appreciate the follow up and looking forward to help 😁 My homeland is a Synology and a NUC, but 🤞 they won't die on me, but I feel your pain. Would most likely build something custom though after 2 years with the Synology. I also understand that IRL sometimes doesn't play nice with hobby projects and FOSS initiatives. Looking forward to the next contribution, but no rush! |
Beta Was this translation helpful? Give feedback.
-
Hi, I have started to break up the code.
https://github.com/Data-Monkey/Pi.Alert/tree/split_it_up
it has lots of global variables and db connections everywhere.
Still lots more work to be done.
Do you have some tests that you are running?
Beta Was this translation helpful? Give feedback.
All reactions