Add new flag to use non-zero exit code when checks fail

[caxu-rh]

• This approach introduces dependency on viper in cli; alternatively, we could add the flag into the CheckConfig struct (but this would require changing how CheckConfig structs are initialized/created in several places). Similarly, the flag could also be added as an additional parameter in RunPreflight, but this would require changing places which expect the existing function signature.
• The failure exit code 1 does not distinguish between "checks did not pass" or "failure occurred while running Preflight (e.g. during image pull); it would not be difficult to use unique exit codes for these two scenarios (e.g. 1 and 2) if that is preferable.

[coveralls]

coverage: 83.252% (-0.4%) from 83.607%
when pulling 99c208e on caxu-rh:optional-exit-failure-code
into 3e6e91f on redhat-openshift-ecosystem:main.

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/8f3fb0b8-e2c9-45e0-8be1-54cac1b07d88/jobStates

[bcrochet]

Alternative...

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/a05528bf-16fd-47d6-ad5e-e41444f6c227/jobStates

[bcrochet]

Great direction, but another suggestion.

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/41067a77-2a7d-451d-a5d2-1e734700a33f/jobStates

[github-actions]

Code Review by Gemini

Here's a review of the provided code changes, focusing on security, best practices, performance, and idiomatic Go.

Overall Impression

The changes introduce a useful feature for controlling the exit code of the preflight tool based on check results. The approach uses idiomatic Go error handling with sentinel errors and errors.Is. The commit message is excellent, providing context, alternatives considered, and a discussion of exit code choices.

Specific Feedback and Suggestions

cmd/preflight/cmd/root.go

1. Flag Definition and Binding:

   • Good: The new flag --exit-with-failure is clearly defined with a helpful description and an associated environment variable PFLT_EXIT_WITH_FAILURE. Binding it to viper is consistent with the project's existing configuration management.
   • Minor Point - Flag Description vs. Implementation: The description for the exit-with-failure flag states: "Exit with exit code 2 if any checks encounter an error or exit code 1 if any checks fail."
     However, the main.go implementation uses:
     • os.Exit(1) for ChecksErroredError (checks encountered an error).
     • os.Exit(2) for ChecksFailedError (checks failed).
       This means the exit codes in the flag description are swapped relative to their actual use.
       Suggestion: Correct the flag description to: "Exit with exit code 1 if any checks encounter an error or exit code 2 if any checks fail."

2. Execute() Function Logic:

   • Good: The use of errors.Is to check for specific error types (ChecksErroredError, ChecksFailedError) is idiomatic and allows for precise control flow. The conditional return based on the exit_with_failure flag correctly implements the desired behavior (swallowing these specific errors if the flag is not set).
   • Minor Point - Error Aliases: The lines type ChecksFailedError = cli.ChecksFailedError and type ChecksErroredError = cli.ChecksErroredError create aliases. While not harmful, main.go directly imports cmd and could use cmd.ChecksErroredError{} without these aliases if cli errors were intended to be exposed through cmd. If cli errors were meant to be internal to cli, then cmd would typically define its own error types that wrap or translate cli errors. Given the current structure, it's a stylistic choice and acceptable.

cmd/preflight/main.go

1. Error Handling and Exit Codes:
   • Good: The main function correctly distinguishes between the custom error types and maps them to specific exit codes (1 for errored, 2 for failed). This is exactly what the feature aims to achieve.
   • Adherence to Commit Body Discussion: The commit body notes: "The failure exit code 1 does not distinguish between 'checks did not pass' or 'failure occurred while running Preflight (e.g. during image pull); it would not be difficult to use unique exit codes for these two scenarios (e.g. 1 and 2) if that is preferable."
     The current implementation uses:
     • os.Exit(1) for ChecksErroredError (checks encountered an error).
     • os.Exit(2) for ChecksFailedError (checks failed).
     • log.Fatal(err) (which calls os.Exit(1)) for other errors (e.g., configuration issues, image pull failures, etc.).
       This means that "checks errored" and "general application errors" both result in exit code 1. This is a conscious design choice as per the commit message. If, in the future, a finer distinction is needed, a unique exit code for "general application errors" could be introduced (e.g., os.Exit(3)). For now, this is acceptable given the explicit discussion.

internal/cli/cli.go

1. Custom Error Types:
   • Good: ChecksFailedError and ChecksErroredError are well-defined as empty structs implementing the error interface. This is an idiomatic Go pattern for sentinel errors, allowing callers to use errors.Is for robust error checking. The Error() methods provide clear, descriptive messages.
2. Error Return in RunPreflight():
   • Good: The checks for results.Errors and results.Failed are placed at the end of the function, ensuring that all other necessary operations (like writing results and submitting them) are completed before an error is returned. This is important for generating complete reports even if checks fail.
   • Good: The order of checks (Errors before Failed) is logical, prioritizing more severe "errored" states over simple "failed" states.

Security Vulnerabilities

No new security vulnerabilities are introduced by these changes. The focus is on error handling and command-line interface behavior.

Performance Optimizations

The changes are in control flow and error handling, not performance-critical sections. No performance optimizations are applicable or needed here.

Idiomatic Go

The code adheres well to idiomatic Go practices:

• Use of sentinel errors with errors.Is.
• Clear function signatures and error returns.
• Consistent use of viper for configuration.

Summary of Key Suggestions

1. Correct the flag description in cmd/preflight/cmd/root.go: Change "Exit with exit code 2 if any checks encounter an error or exit code 1 if any checks fail." to "Exit with exit code 1 if any checks encounter an error or exit code 2 if any checks fail." to match the implementation.

Overall, this is a well-implemented feature with good adherence to best practices and idiomatic Go. The commit message is exemplary in its detail and discussion of design choices.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, caxu-rh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [acornett21]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Code Review by Gemini

The changes introduce a new CLI flag to control the exit code behavior of the preflight tool, providing more granular feedback on the outcome of checks.

Here's a review focusing on the requested aspects:

1. Security Vulnerabilities

No direct security vulnerabilities are introduced by these changes. The modifications are primarily related to command-line flag parsing and error handling, which do not expose new attack surfaces or introduce insecure patterns.

2. Adherence to Best Practices

• Custom Error Types: The introduction of ChecksFailedError and ChecksErroredError as distinct error types is an excellent practice in Go. It allows callers to differentiate between various failure conditions using errors.Is (as demonstrated in cmd/root.go and main.go) rather than relying on string comparisons of error messages. This makes error handling more robust and less brittle.
• Clear Exit Codes: Using distinct exit codes (1 for "errored checks", 2 for "failed checks", and 0 for success) is a best practice for command-line tools. It allows external scripts and automation to easily interpret the outcome of the preflight command without needing to parse output logs. The commit body explicitly mentions this improvement, which is great.
• Separation of Concerns:
  • The internal/cli package is responsible for determining if checks failed or errored and returning the appropriate custom error.
  • The cmd/preflight/cmd package (specifically Execute()) decides whether to propagate these errors based on the exit-with-failure flag.
  • The main.go function is responsible for translating these propagated errors into specific os.Exit codes.
    This separation is clean and maintains good modularity.
• Viper Integration: The use of viper for flag parsing and configuration management is consistent with existing patterns in the codebase and is a standard practice for Go CLI applications.
• Commit Message: The commit message is exceptionally well-written. It clearly explains the problem, the chosen solution, alternatives considered, and the rationale behind the exit code choices. This greatly aids in understanding the changes and their implications.

3. Performance Optimizations

The changes are in the control flow and error handling paths, which are not performance-critical. The impact on performance is negligible and not a concern.

4. Idiomatic Go

• Error Handling: The use of errors.Is with custom error types (empty structs implementing the error interface) is idiomatic Go.
• Flag Definition: Defining CLI flags using cobra and binding them to viper is standard practice for Go CLI applications.
• main Function: The main function's role in handling top-level errors and setting the exit code is idiomatic.
• Error Messages: The Error() methods for the custom error types provide concise and informative messages.

Specific Feedback and Suggestions

1. Clarity of exit-with-failure flag description:
   The description "Exit with exit code 1 if any checks encounter an error or exit code 2 if any checks fail." is very precise and helpful. This is excellent.

2. Order of Error Checks in RunPreflight:
   The order if len(results.Errors) > 0 then if len(results.Failed) > 0 is correct. An "error" (e.g., a check couldn't run due to an image pull failure) is generally a more severe condition than a "failed" check (e.g., a check ran but the image didn't meet the criteria). Prioritizing the ChecksErroredError is the right approach.

3. Consistency in viper key naming:
   The flag is exit-with-failure (kebab-case), and it's bound to viper as exit_with_failure (snake_case). This is a common convention when using viper with CLI flags and is perfectly acceptable.

4. Minor thought on log.Println vs log.Fatal in main.go:
   For ChecksErroredError and ChecksFailedError, you use log.Println(err) followed by os.Exit(code). For other errors, you use log.Fatal(err). log.Fatal internally calls os.Exit(1) after printing the message. This is a stylistic choice, and both approaches are fine. The current approach explicitly shows the different exit codes, which is good.

Overall, this is a well-implemented and thoughtful change that significantly improves the usability and automation capabilities of the preflight tool. The code is clean, follows best practices, and is idiomatic Go.

No critical issues or major improvements are needed.

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/5564fe1f-35c4-4366-bb0f-41130f310298/jobStates

[acornett21]

/test 4.20-e2e

[acornett21]

/test 4.20-images

[acornett21]

/test 4.13-images

[deepsm007]

/test 4.20-images

[deepsm007]

/retest-required

[bcrochet]

A couple of suggestions...

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/9374dbbd-5e38-414d-b5ad-128f12150044/jobStates

[dcibot]

from change #1316:

• SUCCESS https://www.distributed-ci.io/jobs/741d5791-1b6f-464d-91bc-e64d6b491349/jobStates