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

500_make_backup.sh: fix exit code logic #2470

Merged
merged 2 commits into from Aug 7, 2020

Conversation

casantos
Copy link
Contributor

@casantos casantos commented Aug 5, 2020

Ran a backup ensuring that there is not enough space in the destination to write backup.tar.gz.

  • Brief description of the changes in this pull request:

Commit 2674807 removed the BACKUP_PROG_CRYPT_OPTIONS="cat" which breaks
the exit code logic because pipes_rc and backup_prog_shortnames does no
longer match.

Ensure that the number of elements in both variables are alwas the same.

Signed-off-by: Carlos Santos casantos@redhat.com

Commit 2674807 removed the BACKUP_PROG_CRYPT_OPTIONS="cat" which breaks
the exit code logic because pipes_rc and backup_prog_shortnames does no
longer match.

Ensure that the number of elements in both variables are alwas the same.

Signed-off-by: Carlos Santos <casantos@redhat.com>
@jsmeix
Copy link
Member

jsmeix commented Aug 5, 2020

What I like more with your implementation here
compared to my #2472
is that this one here avoids the useless use of cat as dummy.

Now I am wondering if we could also get rid of the useless use of dd
in the other default case when the backup should not be split
with reasonable effort.

On the other hand
#2265
means a complete cleanup of both 500_make_backup.sh
and 400_restore_backup.sh needs to be done at some time anyway.

I wonder if that overall cleanup should be done now or if it is better for now
to not change too much now at once in an hurry and only fix things as they
have been known to work which I tried via #2472

@jsmeix jsmeix requested a review from rmetrich August 5, 2020 14:19
@jsmeix jsmeix self-assigned this Aug 5, 2020
@jsmeix jsmeix added the bug The code does not do what it is meant to do label Aug 5, 2020
@jsmeix jsmeix added this to the ReaR v2.7 milestone Aug 5, 2020
@jsmeix jsmeix requested a review from a team August 5, 2020 14:21
@casantos
Copy link
Contributor Author

casantos commented Aug 5, 2020

What I like more with your implementation here
compared to my #2472
is that this one here avoids the useless use of cat as dummy.

Now I am wondering if we could also get rid of the useless use of dd
in the other default case when the backup should not be split
with reasonable effort.

On the other hand
#2265
means a complete cleanup of both 500_make_backup.sh
and 400_restore_backup.sh needs to be done at some time anyway.

I wonder if that overall cleanup should be done now or if it is better for now
to not change too much now at once in an hurry and only fix things as they
have been known to work which I tried via #2472

I'm not against a deep overhauling but I'd prefer to do it in baby steps.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

Looks good!

@jsmeix jsmeix requested a review from a team August 6, 2020 10:44
@jsmeix
Copy link
Member

jsmeix commented Aug 6, 2020

@rear/contributors
I would like to merge it tomorrow morning provided there are no objections.

Fixed comment about the return code of the backup subshell
and improved some other comments a bit.
@jsmeix jsmeix merged commit ac90051 into rear:master Aug 7, 2020
@jsmeix
Copy link
Member

jsmeix commented Aug 7, 2020

@casantos
thank you for the fix!

@casantos casantos deleted the fix_make_backup_exit branch August 7, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants