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.

Sign up for GitHub

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

Jump to bottom

CI: Use deterministic import order linter/formatter (gci) #3734

Closed
wants to merge 10 commits into from
Closed

CI: Use deterministic import order linter/formatter (gci) #3734

wants to merge 10 commits into from

Conversation

cce
Copy link
Contributor

@cce cce commented Mar 9, 2022 •
edited
Loading

Summary

Uses the gci linter/formatter tool (included in golangci-lint) to fix & enforce breaking imports up into 4 sections: standard libraries, followed by external, followed by algorand-external, followed by local.

Test Plan

Existing tests should pass, reviewdog CI runner that calls golangci-lint should continue to work.

@cce
Copy link
Contributor Author

cce commented Mar 9, 2022

Oh, this is modifying generated msgp code unfortunately.. would need an update on the generator side.

@cce cce added the Enhancement label Mar 9, 2022
@cce cce changed the title Use deterministic import order linter/formatter (gci) CI: Use deterministic import order linter/formatter (gci) Mar 9, 2022
@cce
Copy link
Contributor Author

cce commented Mar 9, 2022

@algojack do you know why https://github.com/algorand/go-algorand/actions/runs/1957829742 reported success even though the reviewdog-errors job in there printed out a linter warning? https://github.com/algorand/go-algorand/runs/5481641372?check_suite_focus=true

tsachiherman
tsachiherman reviewed Mar 9, 2022
View reviewed changes
daemon/algod/api/server/v2/test/handlers_test.go
@@ -333,7 +333,7 @@ func getPendingTransactionsTest(t *testing.T, format string, max uint64, expecte
func TestPendingTransactionLogsEncoding(t *testing.T) {
partitiontest.PartitionTest(t)

response := generated.PendingTransactionResponse{
response := generatedV2.PendingTransactionResponse{
Copy link
Contributor

Choose a reason for hiding this comment

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

this change doesn't look as if it belongs in this PR.

Copy link
Contributor Author

@cce cce Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this was making the linter fail because there were two imports of the same package (one with an alias, the other without) and it was non-deterministic without this update.

tsachiherman
tsachiherman reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Could you apply this change to the msgp first and make the updates there, and only then try to apply this PR ?
btw - I like the PR. It's great that we have automated way to validate the import order.

algorandskiy
algorandskiy reviewed Mar 11, 2022
View reviewed changes
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I like the change but where is the linter itself?
For the generated files (rest api router, msgp) need to modify the generators or add exclusion

@algojack
Copy link
Contributor

@algojack do you know why https://github.com/algorand/go-algorand/actions/runs/1957829742 reported success even though the reviewdog-errors job in there printed out a linter warning? https://github.com/algorand/go-algorand/runs/5481641372?check_suite_focus=true

answered on slack

@cce cce mentioned this pull request Mar 17, 2022
Merged
@cce
Copy link
Contributor Author

cce commented Mar 17, 2022

Updated msgp in algorand/msgp#14 — files it generates now match this PR's changes.

@winder winder mentioned this pull request Mar 29, 2022
Merged
@codecov
Copy link

codecov bot commented Aug 19, 2022 •
edited
Loading

Codecov Report

Merging #3734 (0b9580b) into master (0b9580b) will not change coverage.
The diff coverage is n/a.

❗ Current head 0b9580b differs from pull request most recent head d950ee0. Consider uploading reports for the commit d950ee0 to get more accurate results

@@           Coverage Diff           @@
##           master    #3734   +/-   ##
=======================================
  Coverage   55.24%   55.24%           
=======================================
  Files         398      398           
  Lines       50148    50148           
=======================================
  Hits        27706    27706           
  Misses      20133    20133           
  Partials     2309     2309

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

algorandskiy
algorandskiy reviewed Aug 19, 2022
View reviewed changes
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Looks good but doubled import in one file

data/account/registeryDbOps.go
@@ -24,6 +24,8 @@ import (
"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/protocol"
"strings"

"github.com/algorand/go-algorand/protocol"
Copy link
Contributor

Choose a reason for hiding this comment

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

doubled!

@algojack
Copy link
Contributor

talked to @cce and this PR needs to be updated to newer version of golangci i believe, and tested that everything else still works with our version of go and new version of golangci.

@cce cce mentioned this pull request Jun 7, 2023
Merged
@cce
Copy link
Contributor Author

cce commented Jul 21, 2023

Will bring this back in a new PR against latest

@cce cce closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsachiherman tsachiherman tsachiherman left review comments

@algorandskiy algorandskiy algorandskiy left review comments

@excalq excalq Awaiting requested review from excalq excalq was automatically assigned from algorand/devops

@egieseke egieseke Awaiting requested review from egieseke egieseke was automatically assigned from algorand/devops

@algobarb algobarb Awaiting requested review from algobarb algobarb was automatically assigned from algorand/devops

@algojack algojack Awaiting requested review from algojack algojack was automatically assigned from algorand/devops

@algolucky algolucky Awaiting requested review from algolucky algolucky was automatically assigned from algorand/devops

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cce @algojack @tsachiherman @algorandskiy