-
Notifications
You must be signed in to change notification settings - Fork 12
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
[Linting] fix outstanding linter errors #684
[Linting] fix outstanding linter errors #684
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
pkg/appgateserver/server.go
Outdated
@@ -300,13 +300,13 @@ func (app *appGateServer) ServePprof(ctx context.Context, addr string) error { | |||
// If no error, start the server in a new goroutine | |||
go func() { | |||
app.logger.Info().Str("endpoint", addr).Msg("starting a pprof endpoint") | |||
server.ListenAndServe() | |||
_ = server.ListenAndServe() |
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.
Should we do something here if there's an error?
(cc @red-0ne)
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.
Let's add an error log here. That way, the user/operator can know why the endpoint is not available if something goes wrong (such as a port already in use).
pkg/relayer/relayminer.go
Outdated
@@ -121,13 +121,13 @@ func (rel *relayMiner) ServePprof(ctx context.Context, addr string) error { | |||
// If no error, start the server in a new goroutine | |||
go func() { | |||
rel.logger.Info().Str("endpoint", addr).Msg("starting a pprof endpoint") | |||
server.ListenAndServe() | |||
_ = server.ListenAndServe() |
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.
Should we do something here if there's an error?
(cc @red-0ne)
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.
Let's log the error.
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 684) |
testutil/testproxy/relayerproxy.go
Outdated
go func() { server.ListenAndServe() }() | ||
go func() { | ||
err := server.ListenAndServe() | ||
require.NoError(test.t, err) |
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.
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.
There were a couple of issues:
- We tried to create new proxies with the same config while already having one running for the duration of whole test.
- We did not cleanup the servers properly.
The fix incoming! :)
10f2634
to
70f7bff
Compare
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.
Thank you so much for taking an initiative on this effort!
LGTM, and test seem to pass now.
If you're good with the changes I added - let's merge this in.
Summary
Fix linter errors exposed in #674
Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start
; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_test
make test_e2e
PR Testing (only if making code changes)
devnet-test-e2e
label to the PR.make trigger_ci
if you want to re-trigger tests without any code changesSanity Checklist