[rhcos-4.18] - Backport failing test for FIPS & LUKS#4258
[rhcos-4.18] - Backport failing test for FIPS & LUKS#4258aaradhak wants to merge 1 commit intocoreos:rhcos-4.18from
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request adds a new failing test for FIPS and LUKS, which is a backport. The new test code is well-structured. I've provided a few suggestions to improve code clarity, debuggability, and maintainability. These include using a more descriptive variable name to avoid shadowing, simplifying error handling logic, and improving how errors are reported.
| if err := ignitionFailure(c); err != nil { | ||
| c.Fatal(err.Error()) | ||
| } |
There was a problem hiding this comment.
Passing err directly to c.Fatal() can provide more detailed error information, especially for wrapped errors which may include stack traces. This improves debuggability.
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err.Error()) | |
| } | |
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err) | |
| } |
| case err := <-errchan: | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The logic for handling the error from the channel can be simplified. Returning err directly achieves the same result, as it will be nil on success and contain the error on failure.
case err := <-errchan:
return err| failConfig, err := failConfig.Render(conf.FailWarnings) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "creating invalid FIPS config") | ||
| } | ||
|
|
||
| // Create a temporary log file | ||
| consoleFile := c.H.TempFile("console-") | ||
|
|
||
| // Instruct builder to use it | ||
| builder.ConsoleFile = consoleFile.Name() | ||
| builder.SetConfig(failConfig) |
There was a problem hiding this comment.
The variable failConfig is being shadowed here. It is initially a *conf.UserData at the package level, and then it is redeclared as a *conf.Conf within this function. This can be confusing. It's better to use a different name for the rendered configuration to improve clarity.
renderedConfig, err := failConfig.Render(conf.FailWarnings)
if err != nil {
return errors.Wrapf(err, "creating invalid FIPS config")
}
// Create a temporary log file
consoleFile := c.H.TempFile("console-")
// Instruct builder to use it
builder.ConsoleFile = consoleFile.Name()
builder.SetConfig(renderedConfig)
1 participant
Backporting #4181