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

CI: Fix PartitionTest Github Action #5006

Merged
merged 22 commits into from
Jan 20, 2023

Conversation

algojack
Copy link
Contributor

@algojack algojack commented Jan 11, 2023

Fixing partitionTest linter to work:

  • updated github actions
  • updated tools package version to match golangci-lint (root cause)
  • @cce added a check for nil variable (root cause)
  • broke up the golangci-lint and reviewdog commands to error out if not able to run (secondary cause)
  • added exit code 0 to golangci-lint so that it doesn't fail if it finds something
  • ran go mod tidy -compat=1.17 in cmd/partitiontest_linter

Tested locally and on github actions.

@algojack algojack self-assigned this Jan 11, 2023
@algojack algojack changed the title WIP: Updating action versions WIP: PartitionTest GA Fix Jan 11, 2023
@algojack algojack changed the title WIP: PartitionTest GA Fix WIP: Fix PartitionTest Github Action Jan 11, 2023
@algojack algojack changed the title WIP: Fix PartitionTest Github Action CI: Fix PartitionTest Github Action Jan 13, 2023
@algojack algojack marked this pull request as ready for review January 13, 2023 16:40
@algorandskiy
Copy link
Contributor

updated tools package version to match golangci-lint (root cause)

can we add some checks to ensure golangci-lint build matches to the plugin (part test linter)? Or, I guess why was golangci-lint updated without noticing?

algobarb
algobarb previously approved these changes Jan 13, 2023
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

The code changes relatively seems similar to what was in the PR summary, so I think the changes look good if they've been tested.

@algojack
Copy link
Contributor Author

updated tools package version to match golangci-lint (root cause)

can we add some checks to ensure golangci-lint build matches to the plugin (part test linter)? Or, I guess why was golangci-lint updated without noticing?

Added a fix to error out properly if it doesn't run: broke up the piped commands into two and added set -e and tested on the bugs we had here.

It was updated without noticing because it failed silently (without failing the job)

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #5006 (193a1de) into master (b8866c9) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5006   +/-   ##
=======================================
  Coverage   53.50%   53.51%           
=======================================
  Files         429      429           
  Lines       54038    54038           
=======================================
+ Hits        28911    28916    +5     
+ Misses      22883    22879    -4     
+ Partials     2244     2243    -1     
Impacted Files Coverage Δ
network/wsPeer.go 66.43% <0.00%> (-2.13%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/testing/randomAccounts.go 56.26% <0.00%> (-0.62%) ⬇️
ledger/acctupdates.go 68.75% <0.00%> (-0.49%) ⬇️
network/wsNetwork.go 64.92% <0.00%> (+0.17%) ⬆️
catchup/service.go 71.25% <0.00%> (+2.65%) ⬆️
ledger/tracker.go 78.90% <0.00%> (+3.79%) ⬆️

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

cce
cce previously approved these changes Jan 18, 2023
@cce cce merged commit a6d0231 into algorand:master Jan 20, 2023
@algojack algojack deleted the fixing-partitiontest-check branch January 20, 2023 20:37
@cce cce mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants