[oblvious-earth]

Quickly skimming some points that would irratate me if I had to maintain this script:

* Importing Paramiko but regularly call `ssh` via subprocess

* Unused functions like `execute_network_commands_func`

* Sharing state via a global instead of creating a class

Overall it's fit for purpose, but makes a lot of assumptions about the host and client machines. As you said in the thread you're running a very small number of servers (less than 30). I've written similiar things over the years and they are great for what you need.

When I heavily used Splunk (back in 2013) I was in an application production support team that managed over 100 productions servers for over a dozen applications, there were dozens of other teams in similar situations across the company. The Splunk instance was managed by a central team, minimal assumptions about the client environment, had well defined permissions, understood common and essoteric logging formats, and could reinterpret the log structure at query time. A script like this is not competiting in that kind of situation.

[eigenvalue]

Thanks for the feedback. I do use Paramiko for some things. I tried to use it for everything in the project but ran into some weird stuff that wouldn't work reliably for me, which is why I switched some of it over to using SSH directly via subprocess (it was a few months ago so I don't even remember now what it was; I believe it was also performance related, since I'm trying to SSH to tons of machines at the same time concurrently).

I guess I did forget to use the execute_network_commands_func. I'm using the ruff linter extension in VSCode now which would have flagged that to me, but back when I made this I wasn't.

I don't think globals are so awful for certain things. I prefer a more functional approach where you have simple composable standalone functions instead of classes. Obviously classes have a role, but I find they sometimes overly complicate things and make the logic harder to follow and debug.

Anyway, I do appreciate that someone took the time to actually read through the code!

[oblvious-earth]

> I don't think globals are so awful for certain things. I prefer a more functional approach where you have simple composable standalone functions instead of classes. Obviously classes have a role, but I find they sometimes overly complicate things and make the logic harder to follow and debug.

But "globals" and "composable standalone functions" are contradictory, if you're mutating global state your function is neither composable nor standalone.

What you've got is a poor mans class instance using global instead of self.

[knowsuchagency]

It's a single script. Globals are fine--they're even marked as such.

[oblvious-earth]

> It's a single script

It's over 1200 lines of code, it's not like it's 100 lines of code and can fit on a single screen

> Globals are fine--they're even marked as such

I would argue that globals in this context are not fine from a code maintainability point of view.

By using globals here it's hard to know from a function call if it's going to mutate global state or not. If all the functions were methods of the same class instance, and other functions were just functions or part of some other class, then it gives you a clear grouping of calls which are related to mutating that state.

In general I would argue if you are ever in the situation of "I have more than two or three functions that are related to each other and they all need to mutate the same state so I use a mutable global" or "I pass around the mutable state via arguments" then make a class! It creates an obvious semantic grouping of callables.

[msto]

* barely any comments and not a single docstring in the entire kiloline file

[facorreia]

It's not that much code and it has sensible function names. I appreciate that OP took the time to share his tool with us.

[eigenvalue]

I find comments annoying to read and write and distracting. I’d rather fit more code on the screen at once and instead focus on making the variable names and function names really descriptive and clear so you immediately grasp what it’s doing from context alone. Nowadays, if you really need comments to tell you what code is doing, you can just throw it into ChatGPT and get it that way.

[fastasucan]

I really like the tool you made, and appreciate helping your company save money as well! I don't think it matter that this isn't a perfect fit for everyone else (as you said, this was something you made to solve your problem) - but boy do I disagree with the "variable and function names really descriptive and clear so you immediate grasp what its doing from context alone". What is a descriptive function or variable name is extremely dependent on how familiar you are with the context the program functions in. Using `execute_network_commands_func`from above - this descriptive name say nothing about what network commands that are executed. With docstrings it would be so easy to detail input and output of this function.

[msm_]

I disagree with comments being distracting. You can overdo them, but one good comment - like "baz = 1 # the default is 1 instead of 0, because most real-world production servers foobars 1s into 7s" can save days of frustration.

In general, comments often add a very vaulable context to your code in a way that readaable function/variable names can't.