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

Fixes #27 and corrects comments from pull #27 #31

Merged
merged 5 commits into from
Jun 15, 2018

Conversation

SrZorro
Copy link

@SrZorro SrZorro commented Jun 8, 2018

No description provided.

@@ -13,9 +13,13 @@ cat > ${JOBBER_SCRIPT_DIR}/periodicBackup <<_EOF_

set -o errexit

PREPOSTSTRATEGY=/preexecute/backup
Copy link
Owner

Choose a reason for hiding this comment

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

Magic variable. How and where will this be interpreted?

Better: Two variables DUPLICITY_ACTION, e.g. backup, verify, restore. Then EXECUTION_PHASE, e.g. preAction and postAction.

Variables will be passed directly: prepoststrategy $DUPLICITY_ACTION $EXECUTION_PHASE.

The logic using those variables will be implemented inside prepoststrategy.

This is more maintainable for me.

Copy link
Author

Choose a reason for hiding this comment

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

The magic variable was used inside the prepoststrategy script and was the path to the folder to execute.

But you are right, better with two variables, but instead of declaring the variables in the backup, backupRestore, periodicBackup, etc scripts im going to change it so instead of declaring the variable and then using it in the prepoststrategy script, the prepoststrategy script is going to accept two arguments, execution phase (preAction and postAction) and duplicity action (backup, restore, verify).

This way we remove duplicated code and the user can execute prepost strategies in a more readeable way, example: prepoststrategy preAction verify instead of the old prepoststrategy /preexecute/verify

Copy link
Author

Choose a reason for hiding this comment

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

Last commit should have this fixed.

@blacklabelops blacklabelops merged commit 8a5eb0c into blacklabelops:master Jun 15, 2018
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.

2 participants