ci: add go ci checks, fixed some go vet issues.

[kevwan]

Changes:

1. fixed some go vet issues
2. use gofumpt to format go code

Todo:

1. temporarily disabled go vet check, enable it after issues get fixed

[localvar]

Thanks @kevwan for the PR. I have a question about gofumpt.

Most developers are writing Go code with vscode or goland (myself is using vscode), and gofumpt is not the default formatting tool for vscode (I'm not sure the default formatting tool of goland), this means the introduction of gofumpt may force most Easegress contributors to manually install it before creating a PR.

So, is there any strong justification to switch to gofumpt?

[codecov-commenter]

Codecov Report

Merging #468 (a8becf3) into main (3c21251) will decrease coverage by 0.08%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   80.29%   80.20%   -0.09%     
==========================================
  Files          83       83              
  Lines        9676     9669       -7     
==========================================
- Hits         7769     7755      -14     
- Misses       1464     1469       +5     
- Partials      443      445       +2     

Impacted Files	Coverage Δ
pkg/filter/connectcontrol/connectcontrol.go	96.10% <ø> (ø)
pkg/filter/corsadaptor/corsadaptor.go	90.00% <ø> (-0.25%)	:arrow_down:
pkg/filter/fallback/fallback.go	100.00% <ø> (ø)
pkg/filter/headertojson/headertojson.go	72.54% <ø> (ø)
pkg/filter/kafka/kafka.go	56.73% <ø> (ø)
pkg/filter/mqttclientauth/mqttauth.go	83.56% <ø> (ø)
pkg/filter/proxy/server.go	73.93% <ø> (+0.39%)	:arrow_up:
pkg/filter/topicmapper/topicmapper.go	48.64% <ø> (ø)
pkg/filter/validator/validator.go	96.15% <ø> (ø)
pkg/object/autocertmanager/autocertmanager.go	93.65% <ø> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3c21251...a8becf3. Read the comment docs.

[kevwan]

Thanks @kevwan for the PR. I have a question about gofumpt.

Most developers are writing Go code with vscode or goland (myself is using vscode), and gofumpt is not the default formatting tool for vscode (I'm not sure the default formatting tool of goland), this means the introduction of gofumpt may force most Easegress contributors to manually install it before creating a PR.

So, is there any strong justification to switch to gofumpt?

First of all, for the rules from gofumpt, I think it's a better choice. But do you guys think so? It's the most important thing for this PR. :)

When I write code, gofump almost no errors at all, I think it's a good habit to write clean code with clean coding style.

And if the CI detects the issues, the developers will know how to change the code. I think it's good to let the tool to ask all the contributors to write code in the style that looks great.

Thanks for your reply!

[localvar]

Thanks for the reply, overall, I think gofumpt is better, but my concerns about this change are:

• the default tool is good enough.
• not all contributors know how to check the CI errors, especially for our external contributors.
• change the default tool requires everyone to install the new tool manually.
• not sure if it is configurable, but personally, I prefer using func foo(a string, b string) instead of func foo(a, b string) because all parameters of the former are in the same style which is more friendly for things like grep.

However, the above is just my personal opinion and I'd like to collect more comments from others.

[samutamm]

So is the question which one is better; the current standard go linter gofmt or stricter version of it gofumpt? Personally I am fine with both. I do agree with @localvar that the advantage with the standard gofmt is that you don't have to install it. Also it's more widely adopted and mature.

[kevwan]

Personally, I prefer the stricter one. :)

My thinking is that we need to decide which coding style is preferred. For the tool concern, CI will let contributors know how to do if they don't meet the requirement.

But, I agree with you guys, it's just a choice. Let's pick one.

Cheers.

[suchen-sci]

I am fine with both. But anyone knows why test-win failed? the error is strange...

[mvdan]

not sure if it is configurable, but personally, I prefer using func foo(a string, b string) instead of func foo(a, b string) because all parameters of the former are in the same style which is more friendly for things like grep.

FYI, that rule is opt-in via -extra.

[haoel]

After walking through the code change one by one, I have the following comments, I think some of them are good, and a few are debatable.

• Remove some of the empty lines of code 🆗
• Add the empty line between the functions 🆗
• Add the beginning space for comments 🆗
• Merge the same type of declaration ❓ [not sure]
• Expand the grouped var ( ) declaration which only has one variable 🆗
• Change var name = value to name := value 🆗
• A new line for right brace '}' 👍
• removed the member variables without code reference. ❓ [not sure]

Actually, I think the above changes have a question mark that is not strict, it's a bit obsessive.

And one more question - why does this change break the test on Windows?

[kevwan]

After walking through the code change one by one, I have the following comments, I think some of them are good, and a few are debatable.

• Remove some of the empty lines of code 🆗
• Add the empty line between the functions 🆗
• Add the beginning space for comments 🆗
• Merge the same type of declaration ❓ [not sure]

I agree that it's just a coding style choice. I'll remove -extra to remove this kind of change.

• Expand the grouped var ( ) declaration which only has one variable 🆗
• Change var name = value to name := value 🆗
• A new line for right brace '}' 👍
• removed the member variables without code reference. ❓ [not sure]

I'm thinking that if it's not used, why to keep it?

Actually, I think the above changes have a question mark that is not strict, it's a bit obsessive.

And one more question - why does this change break the test on Windows?

[kevwan]

Because Merge the same type of declaration is not sure, I removed -extra to ignore it.

[localvar]

Besides the test failure on Windows, I think another issue that blocks the merge of the PR is: can we improve the installation experience of the new tool? The most ideal solution is we commit some vscode & goland settings to the repo, and these settings will make the editors install gofumpt automatically and set it as the default tool.

@mvdan , @kevwan , do you have any suggestions on this?

[kevwan]

Besides the test failure on Windows, I think another issue that blocks the merge of the PR is: can we improve the installation experience of the new tool? The most ideal solution is we commit some vscode & goland settings to the repo, and these settings will make the editors install gofumpt automatically and set it as the default tool.

@mvdan , @kevwan , do you have any suggestions on this?

vscode: https://github.com/mvdan/gofumpt#visual-studio-code goland: https://github.com/mvdan/gofumpt#goland