Force --fatal=warnings|errors to honor LOG_FILTER

[ericrdunham]

Pull Request Checklist

• Ensured tests pass and (if applicable) updated functional test output
• Conformed to code style guidelines by running appropriate linting tools
• Added tests for changed code
• Updated documentation for changed code

[avaris]

You can have empty alts, nothing prevents that. pelican will show a warning and if I'm understanding the code correctly, all this does is to turn off that warning. However, you can already do that (or more) with a LOG_FILTER.

[ericrdunham]

@avaris yes, absolutely, but this would allow for a use case where the warnings wouldn't even be emitted, such that --fatal=warnings could be enabled in a CI pipeline without having empty alt text fail it. :)

[avaris]

Ah I see. I can't test right now but if filtered warnings trigger --fatal=warnings then I'd consider that sort of a bug and rather fix that.

[ericrdunham]

@avaris I didn't update the original branch name, but I refactored this now to not add an additional feature and was curious if this was more of what you had in mind?

[avaris]

"Yes" on the idea, "not sure" on the implementation :). I was thinking somewhere along the lines of implementing callHandlers in FatalLogger (instead of warn or error) and raise exceptions there based on the --fatal and record.level.

[ericrdunham]

"Yes" on the idea, "not sure" on the implementation :). I was thinking somewhere along the lines of implementing callHandlers in FatalLogger (instead of warn or error) and raise exceptions there based on the --fatal and record.level.

I'm curious what is seen as the main benefit of that approach (because it's one I haven't seen before)? :)

[avaris]

I'm just not sure about terminating in the middle of handler queue. If more handlers are added after init, they won't get a chance to act on it. Also, I'm not a fan of sys.exiting, but that can be changed :). Raising exception with a message provides better context than abruptly terminating the process.

[ericrdunham]

Given this latest response, I feel like I might be missing the meaning of 'fatal' in the context of Pelican. I was trying to follow:

parser.add_argument('--fatal', metavar='errors|warnings',
                        choices=('errors', 'warnings'), default='',
                        help=('Exit the program with non-zero status if any '
                              'errors/warnings encountered.'))

Immediately exiting seems to be in line with best practices around 'fatal' things in different languages/platforms, so I'm a bit confused.

Right now the existing behavior looks like this when I run it:

$ pelican --settings=samples/pelican.conf.py --output=samples/output/ samples/content/
WARNING: Unable to find '/images/Fat_Cat.jpg', skipping url replacement.
Done: Processed 12 articles, 2 drafts, 3 pages, 1 hidden page and 0 draft pages in 0.30 seconds.

When a LOG_FILTER entry is added:

$ pelican --settings=samples/pelican.conf.py --output=samples/output/ samples/content/ --fatal=warnings
CRITICAL: Warning encountered

Any original messages are dropped and this returns an exit code of 1.

And with the patch it looks like:

$ pelican --settings=samples/pelican.conf.py --output=samples/output/ samples/content/ --fatal=warnings
Done: Processed 12 articles, 2 drafts, 3 pages, 1 hidden page and 0 draft pages in 0.32 seconds.

What do you think about moving forward with this patch to address the one, specific interaction of --fatal with a LOG_FILTER, i.e. a filter entry will not trigger a fatal response?

[avaris]

Maybe I wasn't good at explaining myself. Your PR fixes the issue of --fatal and filtered logs. There is no issue about that. I'm more concerned with the changes to the existing behavior: where a warning is emitted (+ not filtered) and --fatal should terminate. --fatal means exit, no argument there.

I'm talking more about when and how to exit (+ the purpose of logging). I'm assuming when you set --fatal=warnings, yes you want to exit (with non-zero code) but also you want to know what the warning was so that you can fix it. Right now, you're just quitting process while that logging is taking place: one of the logging handlers decides to exit process. If there were more handlers in the queue, they don't get that warning at all. What I'm trying to say was: finish logging that warning first and then exit.

About the sys.exit vs Exception, here is what I was talking about. Currently when --fatal should exit, it looks like this:

$ pelican -s pelicanconf.py --fatal=warnings
WARNING: Empty alt attribute for image logo.png in /home/avaris/ws/test/content/article.md
ERROR: Could not process ./article.md
  | Warning encountered
CRITICAL: Error encountered

and here is your PR

$ pelican -s pelicanconf.py --fatal=warnings
WARNING: Empty alt attribute for image logo.png in /home/avaris/ws/test/content/article.md

First one is more clear. Warning happened, so process shut down. Second one is a bit vague. Warning is written then suddenly process exited. You don't even get a clear picture of "failure" unless you check the exit code. Sure one could look at --fatal=warnings combine with logged warning message... so on. But better to be explicit :).

[ericrdunham]

Ahh, I see. I was approaching it from the other side (getting LOG_FILTER to work with --fatal). :) If I get a non-zero from pelican, I'd be inclined to re-run it with --debug and ignore the bit about:

ERROR: Could not process ./article.md
  | Warning encountered
CRITICAL: Error encountered

I didn't consider that information to be helpful beyond the original WARNING: Empty alt attribute for image logo.png in /home/avaris/ws/test/content/article.md because it didn't give me any additional information about how to fix the problem or where it was coming from beyond the original WARNING line. The basis for this PR was around the --fatal workflow that I've seen, which is to use --fatal and then if a non-zero exit code happens, swap --fatal for --debug and then re-run and dig in to the problem.

Would you prefer that I close this PR and submit this as an issue instead so that more community feedback can be gathered about the desired UX?

[avaris]

I'm still not very happy about sys.exit but this does solve the issue in question, so I'm approving it :), thanks!

[justinmayer]

Thanks for the enhancement, Eric. Would you please have a look at the two minor suggestions I made, and assuming they make sense to you, would you make those changes and rebase & squash your commits?

[justinmayer]

Very minor suggestion, but perhaps change sys.exit(1) to the following?

raise SystemExit(1)

[justinmayer]

Unless this change is somehow related to the other changes in this pull request, I suggest that this change be reverted.

[justinmayer]

Hi @ericrdunham. Is there any chance you'd be willing to resolve the merge conflict here so we can get your Pelican contribution merged? 😊

[justinmayer]

Hi @ericrdunham. Following up again to see if you'd be willing to resolve the merge conflict here so we can get your Pelican contribution merged? 😊

[justinmayer]

Hey Eric. Just following up one last time so we can get your contribution merged. Any chance you could make the above changes and resolve the conflicts here?