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

The 0 exit code is a little misleading #540

Open
drsound opened this issue Feb 13, 2019 · 6 comments

Comments

@drsound
Copy link

commented Feb 13, 2019

As it is implemented right now, the 0 exit code is a little misleading: as it happens for every other program I know, a 0 exit code should be returned if absolutely no problem occurred, not even a warning, so that, for example, you can use it in a script to send an email saying "backup successful".

I just executed a backup, a file was not backed up, but I still got a 0 exit code!

Some questions I asked myself:

  1. What if it was an important file?
  2. What if 99% of the files for some reasons were not backed up? Would I still get a 0 exit code?
  3. Should I always "grep" the backup output looking for signs of trouble?
  4. What if I grep for some specific text but in some future release the warning message changes? I will think my backups are fine but I could miss some files!

I think exit codes were invented to avoid this kind of problems.

My proposal: why not to add another exit code in case of any warning or problem during the backup?

Here is my backup output that gave me 0 exit code, look at the last line:

Backup for / at revision 3 completed
Files: 1278499 total, 573,580M bytes; 183 new, 70,170K bytes
File chunks: 116957 total, 573,807M bytes; 11 new, 62,841K bytes, 19,645K bytes uploaded
Metadata chunks: 92 total, 482,955K bytes; 16 new, 99,754K bytes, 28,975K bytes uploaded
All chunks: 117049 total, 574,278M bytes; 27 new, 162,596K bytes, 48,620K bytes uploaded
Total running time: 00:02:30
1 file was not included due to access errors
@gilbertchen

This comment has been minimized.

Copy link
Owner

commented Feb 14, 2019

Changing the exit code at this time is not a good idea. It would break some existing users' scripts.

I think you can just grep the output to make sure that no files are skipped. These specific log messages are unlikely to be changed in the future.

@Thalagyrt

This comment has been minimized.

Copy link

commented Mar 12, 2019

What about adding an argument that lets us change the exit code behavior? If the argument isn't specified, the behavior stays the same, but if we specify an argument like -detailed-exit-code we could have a more detailed exit code mapping that can tell us if something failed to upload or similar? Grepping the output is really a bad solution here.

@drsound

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

@gilbertchen is afraid of breaking existing backup scripts. I try to analyze the possible cases to show there would be no breaking.

Every kind of backup script I can think of, can do 4 things with the exit code:

  1. Ignore it (Very bad thing!)
  2. Tell the user "everything is fine" if it's 0 (Bad thing! What if you have a lot of missing files?)
  3. Tell the user "everything is fine" if it's 0 AND grepping the output you haven't any "not included files" (Ok, but it leads to ugly and inefficient scripts. Moreover, where in the duplicacy documentation is written you should do this grepping to be sure you have a correct backup? I would bet many people don't do it!)
  4. Tell the user "something went wrong" if it's not 0, with optional variants depending on the exact exit code value

If we add an additional exit code for "not included files" the script behaviour in the previous 4 cases would change as follows:

In case there aren't "not included files" (most common situation):

  1. No change
  2. No change
  3. No change
  4. No change

In case there are "not included files":

  1. No change
  2. Script behaviour would be better, because it will not give the user a false sense of security, a wrong "everything is fine" message
  3. The exit code will be 0 no more, so the grepping will not be done, but every script checking for a 0 exit code, for sure also checks for a non-zero one, so the error will be shown by next case # 4
  4. No change in case the script doen't check the exact non-zero exit code. In case the script checks the exact exit code to show different error messages, the new exit code will not be recognized, so a generic error will be shown.

Now, in my opinion the higher priority should be not to give false sense of security (case # 2).

Anyway, if @gilbertchen does not agree with directly adding a new exit state, I think @Thalagyrt solution is the second best we could have.

@Thalagyrt

This comment has been minimized.

Copy link

commented Mar 12, 2019

Good analysis. I'm definitely in agreement with @drsound here. I don't see any way in which adding a status code for incomplete/failed backups would break existing scripts in a bad way.

If someone's ignoring the exit code, there really is no change. If someone's using set -e or otherwise checking exit codes, then they're going to get an immediate improvement in that their script will know about the failure from the exit code immediately.

Further, if someone's checking exit codes, they pretty much are guaranteed to be knowledgeable enough to have the script send an alert on non-zero exit codes or otherwise just abort on failure and call into something like dead man's snitch on success, and the script aborting on the non-zero would cause DMS or similar to alert since the final check-in would short circuit out.

@jt70471

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Agreed, a return code of zero with a less than successful backup is not good. In my opinion, a non-zero exit code should be used if a file included (explicitly or implicitly) in the backup is not successfully backed up

@ismell

This comment has been minimized.

Copy link

commented Aug 16, 2019

So I'm running running duplicacy web and it shows all my backups as succeeding (they were green). But when I clicked on the backup status I saw a bunch of permission error warnings, so half my data wasn't actually backed up. This is a very scary situation to be in. The exit code needs to indicate these failures.

pylint uses a bit mask as the exit code. That might be useful so the log files don't need to be parsed to determine what type of errors happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.