Inconsistent handling of --dry-run in different configs. #54
Conversation
This change makes how --dry-run is handled more consistent. Signed-off-by: Mark Silva <[email protected]>
|
|
||
| def _offlinecmd(*args: str): | ||
| def _offlinecmd(dry_run: bool, *args: str): |
There was a problem hiding this comment.
Minor. Could these dry_run parameters be keywords at the end of the method prototype that default to False, so that devs don't have to specify _offlinecmd(dry_run,... every time they want to use the method?
There was a problem hiding this comment.
When would you ever not need to pass dry_run in? The whole purpose of the parameter is to not run the command when the --dry-run parameter is passed in on the command line.
There was a problem hiding this comment.
When you're... not doing a dry-run? I don't understand the question.
| @@ -34,6 +34,8 @@ def configure(self, args: argparse.Namespace) -> None: | |||
| logger.debug("Removing unsupported package feeds...") | |||
| if not dry_run: | |||
| subprocess.run(["rm", "-fv", "/etc/opkg/NI-dist.conf"], check=True) | |||
| else: | |||
| print("Dry run: would have run rm -fv /etc/opkg/NI-dist.conf") | |||
There was a problem hiding this comment.
I think it would be more useful to describe the operation in concept, rather than repeating the exact command.
- Users who are issuing the dry run don't necessarily understand what the command string is supposed to do.
- Devs who are trying to debug using the dry-run are going to grep for the output sting and see the real command in the source anyway.
- It would help keep the dry-run output string from falling out of sync w/ the real code.
|
|
||
| run(["nirtcfg", "--set", "section=systemsettings,token=ui.enabled,value=False"], check=True) | ||
| if not args.dry_run: | ||
| run(["nirtcfg", "--set", "section=systemsettings,token=ui.enabled,value=False"], check=True) |
There was a problem hiding this comment.
I have a concern that the nirtcfg binary will not be in the PATH for non-interactive users. Current, the binary is under /usr/local/natinst/..., which is a nonstandard path which gets sourced by interactive logins, but not by non-interactive users (iirc). Could you test this by issuing a call to the configuration tool via a non-interactive ssh shell? ssh -c "nilrt-snac configure --dry-run".
I have an AZDO PR to move nirtcfg into the PATH, but I've had trouble getting linuxpkg to work, so it isn't in yet.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Summary of Changes
Make the handling of
--dry-runconsistent across the different config types.configure()early--dry-run--dry-runinvalidate()if neededJustification
Fixes #46
Testing
PR tests