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

Introduce feature sendIntermediates (define, document, test) #1

Closed
wants to merge 39 commits into from

Conversation

jimklimov
Copy link

Follow-up to discussion at oetiker#455

CC @lotheac @oetiker

…st and fix more typos in comments and whitespaces
…non-exiting calls and a die() with better error context
…on that is expected to work vs. complicated setup that is not (so far)
…g tank/source/dest-disabled, it confuses other code at the moment
@lotheac
Copy link
Owner

lotheac commented Nov 22, 2019 via email

@jimklimov
Copy link
Author

To inherit your commit, and eventually if you merge - the linked discussion at your original PR for the feature :)

@oetiker
Copy link

oetiker commented Nov 22, 2019

I like this, but the default should not change .. so maybe better have skipIntermediates. We use the -I feature to make sure that in case a send operation takes too long, we still get all the intermediate snapshots sent to the destionation ...

Allow to set dst_N_enabled=off to comment away targets without deleting definitions
@jimklimov
Copy link
Author

No problem with this on my side (even better so, per least-surprise principle). But Lauri may get upset :)

…nal default in place (maybe safer on side of caution, and least-surprise for existing deployments and users)
@jimklimov
Copy link
Author

So updated the code and docs to revive the upstream's default. Also noted that for regular background znapzend services it may be beneficial to set this feature, at least in certain conditions (unsent snaps are not modus vivendi but a rare exception). Since this situation is not ubiquitous, not proposing changes into packaged pre-sets for the services :)

@jimklimov
Copy link
Author

Hm... seems travis is not enabled in this fork :) I'll resubmit as PR to upstream...

@lotheac
Copy link
Owner

lotheac commented Nov 22, 2019 via email

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.

3 participants