Code review comment for lp:~fo0bar/turku/turku-agent-gonogo

Revision history for this message
Ryan Finnie (fo0bar) wrote :

> Could you explain a little more about what this change is doing, ideally in
> the help for the command and comments in the code?

> Looks like there's a fair amount of techdebt here in terms of lack of lint or
> tests. I don't have enough context on the change to understand whether it
> makes sense to block on those or have those happen as a separate change. Could
> you explain why this is needed and what the impact is?

Change has been updated. Yeah, the codebase could use some large QoL updates, but in a nutshell, the go/no-go program is to prevent pings (and ultimately backups) during sensitive times as determined on the agent machine itself.

> It seems a little confusing to have a variable called "gonogo_program" that's
> actually a list.

Done. The existing command line arg remains --gonogo-program but I've documented it, and kept etc config as gonogo_program to be consistent with the arg, but I've made them loose enough to either be a list (program + args) or string (shlex-split). The post-processed variable has been changed to gonogo_program_and_args to avoid confusion.

> And if I'm understanding this right, this option can be set either via the
> command line option or via a config option on disk? I'm not entirely sure, but
> it looks like that's the case from the change to turku_agent/

Yeah. Existing --gonogo-program, but I wanted to add the on-disk etc option so you didn't have to edit a cron file or systemd timer (it was the only thing you couldn't do through etc, and the .cron and .timer files were essentially part of the software package). Added help to --gonogo-program.

This repo could definitely use some documentation, or at least a README.

« Back to merge proposal