Conversation
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #894caf
Actionable Suggestions - 1
-
test/e2e/framework/helper/poll.go - 1
- Network connection not properly closed on error · Line 134-142
Additional Suggestions - 7
-
test/e2e/suite/cases/upgrade/upgrade.go - 1
-
Using context.TODO() instead of proper context · Line 135-135The code is replacing `exec.Stream(sopt)` with `exec.StreamWithContext(context.TODO(), sopt)` which is good, but using `context.TODO()` is not ideal. Consider using a proper context from the test function or creating one with timeout.
Code suggestion
@@ -134,2 +134,3 @@ ginkgo.By("Running exec into pod and runing curl on local host") - err = exec.StreamWithContext(context.TODO(), sopt) + ctx, cancel := context.WithTimeout(context.Background(), time.Second*30) + defer cancel() + err = exec.StreamWithContext(ctx, sopt)
-
-
test/e2e/framework/util.go - 1
-
Incorrect context type for long-running operation · Line 39-46The `ctx` variable is created with `context.TODO()` but should use `context.Background()` for long-running operations like polling. `TODO()` is intended for temporary placeholders during development.
Code suggestion
@@ -39,1 +39,1 @@ - ctx := context.TODO() + ctx := context.Background()
-
-
test/e2e/suite/cases/impersonation/impersonation.go - 1
-
Semantic duplication in ginkgo/gomega import aliases · Line 9-10Removing dot imports (`.`) but still using the same package names creates semantic duplication. The code now has both `ginkgo` and `gomega` as explicit imports but still uses them with the same names in the code.
Code suggestion
@@ -9,2 +9,2 @@ - ginkgo "github.com/onsi/ginkgo" - gomega "github.com/onsi/gomega" + g "github.com/onsi/ginkgo" + o "github.com/onsi/gomega"
-
-
test/e2e/framework/helper/poll.go - 2
-
Redundant timeout in polling function call · Line 22-25The code creates a context with timeout and then passes the same timeout to `PollUntilContextTimeout`, which is redundant. The context already has the timeout, so passing it again to the polling function is unnecessary.
Code suggestion
@@ -22,5 +22,5 @@ ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - err := wait.PollUntilContextTimeout(ctx, 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 0, true, func(ctx context.Context) (bool, error) {
-
Redundant timeout in pod ready polling function · Line 49-52Same redundant timeout issue in `WaitForPodReady` function. The context already has a timeout, so passing the same timeout to `PollUntilContextTimeout` is unnecessary.
Code suggestion
@@ -49,5 +49,5 @@ ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - err := wait.PollUntilContextTimeout(ctx, 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 0, true, func(ctx context.Context) (bool, error) {
-
-
test/kind/kind.go - 2
-
Redundant timeout in PollUntilContextTimeout function call · Line 221-225The code creates a context with timeout but then also passes a timeout duration to `PollUntilContextTimeout`. This creates redundant timeout mechanisms as the function already uses the context timeout.
Code suggestion
@@ -221,5 +221,5 @@ - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) - defer cancel() - - return wait.PollUntilContextTimeout(ctx, 5*time.Second, 10*time.Minute, true, func(ctx context.Context) (bool, error) { + ctx := context.Background() + + + return wait.PollUntilContextTimeout(ctx, 5*time.Second, 10*time.Minute, true, func(ctx context.Context) (bool, error) {
-
Redundant timeout in context and poll function · Line 265-269The code creates a context with timeout but then also passes a separate timeout to `PollUntilContextTimeout`. This creates redundant timeout mechanisms as the context timeout and poll timeout are both set to 10 minutes.
Code suggestion
@@ -265,7 +265,7 @@ func (k *Kind) waitForPodsReady(namespace, labelSelector string) error { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() - return wait.PollUntilContextTimeout(ctx, 5*time.Second, 10*time.Minute, true, func(ctx context.Context) (bool, error) { + return wait.PollUntilContextTimeout(ctx, 5*time.Second, 0, true, func(ctx context.Context) (bool, error) { pods, err := k.client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ LabelSelector: labelSelector, })
-
Review Details
-
Files reviewed - 28 · Commit Range:
52e37bc..a34776e- cmd/app/options/options.go
- pkg/proxy/context/context.go
- pkg/proxy/handlers.go
- pkg/proxy/namespace_round_tripper.go
- pkg/proxy/proxy.go
- pkg/proxy/proxy_test.go
- pkg/proxy/subjectaccessreview/subjectaccessreview_test.go
- pkg/util/port.go
- test/e2e/framework/framework.go
- test/e2e/framework/helper/poll.go
- test/e2e/framework/helper/requester.go
- test/e2e/framework/helper/secrets.go
- test/e2e/framework/util.go
- test/e2e/suite/cases/audit/audit.go
- test/e2e/suite/cases/headers/headers.go
- test/e2e/suite/cases/impersonation/impersonation.go
- test/e2e/suite/cases/passthrough/passthrough.go
- test/e2e/suite/cases/probe/probe.go
- test/e2e/suite/cases/rbac/rbac.go
- test/e2e/suite/cases/token/token.go
- test/e2e/suite/cases/upgrade/upgrade.go
- test/e2e/suite/suite.go
- test/kind/image.go
- test/kind/kind.go
- test/tools/audit-webhook/pkg/sink/sink.go
- test/tools/fake-apiserver/pkg/server/server.go
- test/tools/issuer/pkg/issuer/issuer.go
- test/util/tls.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
| con, err := net.DialTimeout("tcp", host, timeout) | ||
| if err != nil { | ||
| return false, nil | ||
| } else { | ||
| con.Close() | ||
| defer func() { | ||
| _ = con.Close() | ||
| }() | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
The connection to the URL is not properly closed if there's an error. Add a defer to close the connection after checking for errors to prevent resource leaks.
Code suggestion
Check the AI-generated fix before applying
@@ -134,11 +134,11 @@
+ con, err := net.DialTimeout("tcp", host, timeout)
+ if err != nil {
+ return false, nil
+ } else {
+ defer func() {
+ _ = con.Close()
+ }()
+ return true, nil
+ }
@@ -134,11 +134,12 @@
-
con, err := net.DialTimeout("tcp", host, timeout) -
if err != nil { -
return false, nil -
} -
defer func() { -
_ = con.Close() -
}() -
return true, nil
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/platform9/pf9-oidc-proxy/pull/14#issuecomment-3200886537>#894caf</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
indradhanush
left a comment
indradhanush
left a comment
There was a problem hiding this comment.
Left a bunch of comments about not ignoring the error. I'm surprised the linter didn't catch this already. Can you check how to enforce this check with golangci-lint 2.x?
Not blocking, because lint fixes are still good to have!
| cols, _, _ := term.GetSize(0) | ||
| cmd.SetUsageFunc(func(cmd *cobra.Command) error { | ||
| fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine()) | ||
| _, _ = fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine()) |
There was a problem hiding this comment.
Don't ignore the error being returned. You can ignore n.
So something like this:
| _, _ = fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine()) | |
| if _, err := fmt.Fprintf(cmd.OutOrStderr(), usageFmt, cmd.UseLine()); err != nil { | |
| return err | |
| } |
|
|
||
| cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) { | ||
| fmt.Fprintf(cmd.OutOrStdout(), "%s\n\n"+usageFmt, cmd.Long, cmd.UseLine()) | ||
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s\n\n"+usageFmt, cmd.Long, cmd.UseLine()) |
| "k8s.io/apiserver/pkg/authentication/user" | ||
| "k8s.io/apiserver/pkg/endpoints/request" | ||
| "k8s.io/client-go/transport" | ||
|
|
||
| genericapirequest "k8s.io/apiserver/pkg/endpoints/request" |
There was a problem hiding this comment.
Instead of removing this, why not remove genericapirequest? Then you won't have to make code changes related to this import. There's no conflict with request at the moment, so we can directly import it as is.
| } | ||
| defer file.Close() | ||
| defer func() { | ||
| _ = file.Close() |
There was a problem hiding this comment.
Don't ignore the error. Log it.
| } | ||
| defer watcher.Close() | ||
| defer func() { | ||
| _ = watcher.Close() |
| } else { | ||
| con.Close() | ||
| defer func() { | ||
| _ = con.Close() |
There was a problem hiding this comment.
Don't ignore the error.
hsri-pf9
force-pushed
the
private/harsh/lint-fix
branch
from
a34776e to
3bf4de7
Compare
|
I will resolve the comments in this PR |
Code Review Agent Run #9e5c68Actionable Suggestions - 0Additional Suggestions - 6
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
indradhanush
left a comment
indradhanush
left a comment
There was a problem hiding this comment.
Lint is still failing. The lint failures looked very suspicious to me so I looked deeper. You'll need to:
- Run
go mod tidy - Run the lint again locally to see the failures which will be the correct ones this time
2 participants
Stacked on PR #13
Summary by Bito
This pull request implements comprehensive lint fixes and code modernization throughout the codebase. It replaces deprecated functions and dot imports with explicit ginkgo and gomega calls, while updating outdated I/O operations with modern alternatives. The changes enhance error handling, resource management, and test reliability across build scripts and core proxy functionalities.