Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement --dryrun argument #748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

anarcat
Copy link

@anarcat anarcat commented May 30, 2022

This is a little messy: that includes a lot of whitespace change,
but I couldn't really figure out a better way to do it, in general.

(A better way would have been something like:

if ($args{'dryrun'}) return 0;

... but the functions are generally too long for that kind of hack to
work at all. Alternatively, I figured it might be good to have a
generic system() wrapper, but I haven't dug deep enough in the code
to see if it's always called the same way, or if that would make
sense.)

This doesn't block all system() calls either: only those who
actually make changes. SSH setup commands and commands that just probe
the snapshot lists are let through, so this is not completely a
--noop flag (hence the --dryrun instead), because it
actually does some things still.

I have only grepped for system() and haven't audited the entire
source code to see if other side effects (e.g. creating files) also
exist.

Disclaimer: this patch was briefly tested on my home system, and my
level of familiarity with sanoid is "beginner".

Closes: #11

This is a little messy: that includes a *lot* of whitespace change,
but I couldn't really figure out a better way to do it, in general.

(A better way would have been something like:

    if ($args{'dryrun'}) return 0;

... but the functions are generally too long for that kind of hack to
work at all. Alternatively, I figured it might be good to have a
generic `system()` wrapper, but I haven't dug deep enough in the code
to see if it's always called the same way, or if that would make
sense.)

This doesn't block *all* `system()` calls either: only those who
actually make changes. SSH setup commands and commands that just probe
the snapshot lists are let through, so this is not completely a
`--noop` flag (hence the `--dryrun` instead), because it
actually *does* some things still.

I have only grepped for `system()` and haven't audited the entire
source code to see if other side effects (e.g. creating files) also
exist.

Disclaimer: this patch was briefly tested on my home system, and my
level of familiarity with sanoid is "beginner".

Closes: jimsalterjrs#11
Copy link
Collaborator

@phreaker0 phreaker0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you guessed, the white space issue needs to be resolved.
Did you test you change extensively because I guess there will be error's in edge cases for example with clone handling and such stuff.

@anarcat
Copy link
Author

anarcat commented Mar 21, 2023

i didn't test this very much, just a few trial runs. this is mostly a proof-of-concept I guess, looking for feedback whether this is a desirable feature at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syncoid Dry Run Options
2 participants