Skip to content

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

Merged
merged 12 commits into from
Jun 2, 2025
Merged

upgrade bazel to 8 #58

merged 12 commits into from
Jun 2, 2025

Conversation

apesternikov
Copy link
Contributor

@apesternikov apesternikov commented May 16, 2025

  • upgrade bazel to 8
  • remove go vendoring
  • modernize go library naming in a compatible way

@apesternikov apesternikov changed the title Ap/upgrade bazel upgrade bazel to 8 May 16, 2025
Comment on lines +45 to 46
body, _ := io.ReadAll(response.Body)
if !strings.Contains(string(body), "Hello World") {

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))

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))
}

Comment on lines +41 to 42
`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

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.

Comment on lines 92 to 97
log.Fatal(fmt.Errorf("unable to read setup command STDOUT; %w", err))
}


go func() {
rd := bufio.NewReader(s.er)
for {

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.

Comment on lines 132 to 141
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 {

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.

@apesternikov apesternikov marked this pull request as ready for review May 20, 2025 00:24
@apesternikov apesternikov merged commit a814a45 into main Jun 2, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant