-
-
Notifications
You must be signed in to change notification settings - Fork 107
fix: backup-unset-public-key-encryption command #336
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
base: master
Are you sure you want to change the base?
Conversation
| local SERVICE_ROOT="${PLUGIN_DATA_ROOT}/${SERVICE}" | ||
| local SERVICE_BACKUP_ENCRYPTION_ROOT="${SERVICE_ROOT}/backup-encryption/" | ||
|
|
||
| mkdir "$SERVICE_BACKUP_ENCRYPTION_ROOT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added '-p' flag to prevent this command from failing if the encryption directory already exists. This will occur after an encryption type was set and then removed.
| rm "$SERVICE_BACKUP_ENCRYPTION_ROOT/ENCRYPTION_KEY" | ||
| } | ||
|
|
||
| service_backup_unset_encryption() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a duplicate of the above function prior to this change.
| [[ -z "$SERVICE" ]] && dokku_log_fail "Please specify a valid name for the service" | ||
| verify_service_name "$SERVICE" | ||
| service_backup_unset_public_key_encryption "$SERVICE" # TODO: [22.03.2024 by Mykola] | ||
| service_backup_unset_public_key_encryption "$SERVICE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't pointing to an actual function and would blow up at this point prior to the above change in common-functions.
| [[ ${argv[0]} == "$cmd" ]] && shift 1 | ||
| declare SERVICE="$1" | ||
| is_implemented_command "$cmd" || dokku_log_fail "Not yet implemented" # TODO: [22.03.2024 by Mykola] | ||
| is_implemented_command "$cmd" || dokku_log_fail "Not yet implemented" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up todos
|
Anything I can do to help get this merged? |
README.md
Outdated
| If both passphrase and public key forms of encryption are set, the public key encryption will take precedence. | ||
|
|
||
| The underlying core backup script is present [here](https://github.com/dokku/docker-s3backup/blob/main/backup.sh). | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readme is autogenerated, so you'll need to update the doc generation to get this into place as it will be wiped on next generation.
You can use the bin/generate script to regenerate the readme to test your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the generation script and moved a couple of the comments into the description variables so they'd appear with the specific method info. Thee desc lines are a bit longer now, so let me know if you'd like me to make any stylistic changes or shift the location of the info -- maybe putting it all at the top level where I made the generation script edits.
dbf99bf to
c9f2e6e
Compare
|
@josegonzalez anything else I can do to help get this merged upstream? |
The "backup-unset-public-key-encryption" is not currently functioning. These changes resolve that.
I updated the set / unset functions to enable removing either the public-key-encryption or the encryption-passphrase in isolation without removing the other and added some notes to the readme.