-
Notifications
You must be signed in to change notification settings - Fork 16
upgrade bazel to 8 #58
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
Conversation
body, _ := io.ReadAll(response.Body) | ||
if !strings.Contains(string(body), "Hello World") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from io.ReadAll(response.Body)
is ignored, which can lead to unhandled errors if reading the response body fails. It's recommended to handle this error to ensure the test does not pass incorrectly if an I/O error occurs.
Recommended Change:
body, err := io.ReadAll(response.Body)
if err != nil {
t.Fatalf("Failed to read response body: %v", err)
}
@@ -42,7 +42,7 @@ func TestSimpleServer(t *testing.T) { | |||
if response.StatusCode != 200 { | |||
t.Errorf("Expected status code 200, got %d", response.StatusCode) | |||
} | |||
body, _ := ioutil.ReadAll(response.Body) | |||
body, _ := io.ReadAll(response.Body) | |||
if !strings.Contains(string(body), "Hello World") { | |||
t.Error("Unexpected content returned:", string(body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message in t.Error
does not provide detailed information about what was actually received when the test fails, which can make debugging more difficult. Including the actual content received in the error message would be more helpful for diagnosing issues.
Recommended Change:
if !strings.Contains(string(body), "Hello World") {
t.Errorf("Unexpected content returned: %s", string(body))
}
`WORKSPACE` file is not supported in this version. The latest version with `WORKSPACE` support is [v0.32.13](https://github.com/fasterci/rules_gitops/releases/tag/v0.32.13) | ||
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script embeds user-facing documentation about Bazel usage and version support directly within operational code. This mixing of concerns can lead to maintenance challenges as changes in functionality might require updates to embedded documentation, which could be overlooked.
Recommendation: Consider separating operational code from user-facing documentation. Documentation could be moved to a separate markdown file or a dedicated documentation section within the repository. This separation would enhance clarity and maintainability.
log.Fatal(fmt.Errorf("unable to read setup command STDOUT; %w", err)) | ||
} | ||
|
||
|
||
go func() { | ||
rd := bufio.NewReader(s.er) | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of log.Fatal
in line 92 for error handling is problematic as it causes the program to terminate immediately, potentially skipping necessary cleanup or more graceful error handling. Consider using a less severe logging level and returning an error to allow the caller to decide on further actions. Additionally, the goroutine initiated in line 95 reads from the error stream indefinitely without clear exit conditions, which could lead to resource leaks or deadlocks if not managed correctly. Ensure that there are conditions under which the goroutine can exit cleanly and that errors are handled appropriately within the goroutine.
if strings.HasPrefix(str, "FORWARD") { | ||
// remove the "FORWARD " prefix, and any trailing space, split on ":" | ||
parts := strings.Split(strings.TrimSpace(str[8:]), ":") | ||
localPort, _ := strconv.Atoi(parts[2]) | ||
localPort, err := strconv.Atoi(parts[2]) | ||
if err != nil { | ||
log.Fatalf("Unable to parse local port '%s' from setup script stdout. Cannot wait for pods", parts[2]) | ||
} | ||
s.forwards[parts[0]] = localPort | ||
} | ||
if "READY\n" == str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsing logic in lines 134-135 assumes a specific format for the string being split, which could lead to runtime errors if the format is unexpected. This is risky as it could cause the application to crash due to an unhandled error. Use more robust error checking and handling around the string parsing. Additionally, the use of log.Fatalf
in line 137 for handling parsing errors is severe, as it terminates the program. Consider logging the error and continuing with a default behavior or a retry mechanism, which could make the system more resilient to input variations or errors.
Uh oh!
There was an error while loading. Please reload this page.