Skip to content
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

[sdk50] Lint the go relayer like IBC-go #1274

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Aug 30, 2023

This PR lints the go relayer in the same manner as ibc-go before another PR to upgrade to sdk 50 and ibc8

@@ -40,14 +41,15 @@ func StartDebugServer(ctx context.Context, log *zap.Logger, ln net.Listener, reg
mux.Handle("/relayer/metrics", promhttp.HandlerFor(registry, promhttp.HandlerOpts{}))

srv := &http.Server{
Handler: mux,
ErrorLog: zap.NewStdLog(log),
ReadHeaderTimeout: 5 * time.Second,
Copy link
Contributor Author

@faddat faddat Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain that the ReadHeaderTimeout value is set correctly. We may wish to update it.

@faddat faddat closed this Sep 1, 2023
@faddat faddat reopened this Sep 1, 2023
@jtieri
Copy link
Member

jtieri commented Nov 14, 2023

hey @faddat as of today we have officially merged the changes necessary to upgrade the relayer to ibc-go v8, cometbft v0.38.0 and sdk v0.50.1, (see #1312 where I gave you a shoutout), thanks for all your help on that!

should we take another stab at getting the golangci configuration in line with the ibc-go repo's? if you are still interested in taking on this task my only ask would be that we break the PR up into a series of smaller PRs that are easier to review.

I'm thinking maybe something like this:

  • one PR to introduce the changes to the golangci.yml file along with the GitHub action workflow file (lint.yml) and the changes to the Makefile
  • then maybe a separate PR that addresses each type of linter error? for example if you run the linter and there are a bunch of errors with SA1019: maybe make one PR that just fixes all of those issues.

Sorry if this sounds like a lot but it's really hard to give a thorough review when there are so many changes across so many files and I want to be sure that I can provide a solid review for your hard work!

I haven't actually tried linting the repo with the updated golangci.yml file here so I'm not sure how many issues it turns up, perhaps what i'm asking for is completely unreasonable if there are like 100 different types of linter errors. In that case feel free to address a bunch of them in one PR, but it would be nice if we could keep each PR limited to like 250 LOC changes.

As always thanks for your contributions! If you no longer wanna take this on i can cherry-pick some of your commits into a new branch and give you credit, just let me know 🙂

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.

None yet

2 participants