[rhcos-4.19] - Backport failing test for FIPS & LUKS #4257
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
This pull request backports a failing test for FIPS and LUKS. The new test verifies that cryptsetup fails as expected when using a FIPS-incompatible algorithm. The code is well-structured, but I have a few suggestions to improve clarity and conciseness.
| if err := ignitionFailure(c); err != nil { | ||
| c.Fatal(err.Error()) | ||
| } |
There was a problem hiding this comment.
It's more idiomatic to pass the error object directly to c.Fatal() instead of err.Error(). The testing harness will handle formatting it as a string, and this preserves the error type information.
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err.Error()) | |
| } | |
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err) | |
| } |
| func fileContainsPattern(path string, searchPattern string) (bool, error) { | ||
| file, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| // File has content, but the pattern is not present | ||
| match := regexp.MustCompile(searchPattern).Match(file) | ||
| if match { | ||
| // Pattern found | ||
| return true, nil | ||
| } | ||
| // Pattern not found | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This function can be simplified. The comments are not very helpful, and using regexp.Match would be more concise and idiomatic Go. regexp.Match directly returns (bool, error), which matches the function signature, and handles potential regexp compilation errors gracefully unlike regexp.MustCompile which would panic.
func fileContainsPattern(path string, searchPattern string) (bool, error) {
file, err := os.ReadFile(path)
if err != nil {
return false, err
}
return regexp.Match(searchPattern, file)
}| 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 shadowed inside this function. The global failConfig is a *conf.UserData, while the local failConfig returned by failConfig.Render() is a *conf.Conf. This can be confusing. Renaming the local variable would 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)
aaradhak
changed the title
|
@aaradhak: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
backporting #4181