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

CheckWriteHook: Don't allow Noop action #1089

Merged
merged 2 commits into from Apr 29, 2022
Merged

CheckWriteHook: Don't allow Noop action #1089

merged 2 commits into from Apr 29, 2022

Commits on Apr 29, 2022

  1. CheckWriteHook: Don't allow Noop action

    In #1088, we deprecated the CheckWriteAction enum in favor of the new
    fully customizable CheckWriteHook. Unfortunately, this introduced a
    minor regression: it's now possible to set log.Fatal to no-op with
    WriteThenNoop.
    
    Such a configuration will break code that looks like the following:
    
    ```go
    f, err := os.Open(..)
    if err != nil {
        log.Fatal("Cannot open file", zap.Error(err))
    }
    
    // Control flow expects that if we get here,
    // f is valid and non-nil.
    // That's not the case if Fatal no-ops.
    
    fmt.Println(f.Name())
    ```
    
    This change fixes the regression by turning Noops into WriteThenFatal
    when logging Fatal log messages. This matches the old behavior.
    
    It further clarifies the documentation for CheckWriteHook so that users
    know to call runtime.Goexit or os.Exit in them. It's still possible to
    write a custom hook that no-ops the log.Fatal, but it requires a little
    extra effort from the user to do so.
    abhinav committed Apr 29, 2022
    Configuration menu
    Copy the full SHA
    3baaae2 View commit details
    Browse the repository at this point in the history
  2. Revert formatting change

    abhinav committed Apr 29, 2022
    Configuration menu
    Copy the full SHA
    ba9689d View commit details
    Browse the repository at this point in the history