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

[Automation] Add CI linters/checks #190

Merged
merged 19 commits into from
Oct 6, 2022
Merged

[Automation] Add CI linters/checks #190

merged 19 commits into from
Oct 6, 2022

Conversation

okdas
Copy link
Member

@okdas okdas commented Sep 2, 2022

Description

Adding linters and static checkers to CI rotation.

Issue

CI part of #175

Type of change

Please mark the options that are relevant.

  • New feature, functionality or library
  • Bug fix (non-breaking change which fixes an issue)
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other: <REPLACE_ME>

List of changes

  • Adding more checks for CI

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • If applicable, I have made corresponding changes to related local or global README
  • If applicable, I have added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

@okdas okdas added priority:medium tooling tooling to support development, testing et al labels Sep 2, 2022
@okdas okdas added this to the M1: Pocket PoS (Proof of Stake) milestone Sep 2, 2022
@okdas okdas self-assigned this Sep 2, 2022
@okdas okdas requested a review from a team as a code owner September 2, 2022 00:09
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@58bc07f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #190   +/-   ##
=======================================
  Coverage        ?   40.46%           
=======================================
  Files           ?       26           
  Lines           ?     1913           
  Branches        ?        0           
=======================================
  Hits            ?      774           
  Misses          ?     1082           
  Partials        ?       57           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Please update the PR description.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
.github/workflows/linters.yml Outdated Show resolved Hide resolved
@okdas okdas changed the title [Automation] Add CI linters/checks [Automation] Add CI linters/checks wip Sep 6, 2022
@Olshansk Olshansk marked this pull request as draft September 6, 2022 17:39
@Olshansk Olshansk changed the title [Automation] Add CI linters/checks wip [Automation] Add CI linters/checks Sep 6, 2022
@okdas okdas linked an issue Sep 6, 2022 that may be closed by this pull request
12 tasks
@Olshansk
Copy link
Member

Olshansk commented Sep 8, 2022

@okdas We might be doing a techdebt "bug bash" next week. Do you think it'd be possible to have a baseline implementation of this that we can build on / add to?

@okdas
Copy link
Member Author

okdas commented Sep 8, 2022

Do you think it'd be possible to have a baseline implementation of this that we can build on / add to?

Yes, if this is not going to be ready by then I will export linters output we can go through together.

@Olshansk
Copy link
Member

@okdas Just wanted to follow up if there are any blockers on this PR?

@okdas
Copy link
Member Author

okdas commented Sep 19, 2022

@okdas Just wanted to follow up if there are any blockers on this PR?

No blockers, will pick it up soon.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

I think you can change this from a draft to a real PR.

docs/releases/README.md Show resolved Hide resolved
docs/releases/README.md Outdated Show resolved Hide resolved
docs/releases/README.md Outdated Show resolved Hide resolved
docs/releases/README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docs/development/README.md Outdated Show resolved Hide resolved
docs/development/README.md Show resolved Hide resolved
@Olshansk
Copy link
Member

@okdas Do you need another review on this or is it still a WIP?

@okdas okdas marked this pull request as ready for review September 27, 2022 01:06
@okdas
Copy link
Member Author

okdas commented Sep 27, 2022

@okdas Do you need another review on this or is it still a WIP?

this is ready for review now, thanks!

misc/linting-rules/tests.go Outdated Show resolved Hide resolved
misc/linting-rules/tests.go Outdated Show resolved Hide resolved
misc/linting-rules/tests.go Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docs/development/README.md Show resolved Hide resolved
docs/development/README.md Outdated Show resolved Hide resolved
docs/releases/README.md Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Left a few some suggestions/comments throughout but looking forward to getting this in!

Very well designed and documented!

docs/development/README.md Outdated Show resolved Hide resolved
docs/development/README.md Outdated Show resolved Hide resolved
docs/releases/README.md Show resolved Hide resolved
docs/releases/README.md Show resolved Hide resolved
@Olshansk
Copy link
Member

@okdas Lmk when I should re-review here.

Someone on the team will likely need to a single followup PR because it looks like we caught a bunch of stuff :)

Screen Shot 2022-09-29 at 5 50 16 PM

@okdas
Copy link
Member Author

okdas commented Sep 30, 2022

@Olshansk you can re-review now.

Someone on the team will likely need to a single followup PR because it looks like we caught a bunch of stuff :)

Haha, yes!

Here's an example of golangci-lint run --fix changes: https://github.com/pokt-network/pocket/compare/dk-add-linters...dk-add-linters-autofix?expand=1 - some of the issues can be fixed automatically.

Note: we have a very limited number of linters turned on! If you want to go crazy you could run golangci-lint run --enable-all --fix - just beware this changes lots of files, some linters can be redundant, and in order to run this command you'd need to comment out enabled: [] section in .golangci.yml file.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@okdas

  1. Next time, please make sure to "re-request review" when something is ready to be re-reviewed:

Screen Shot 2022-10-03 at 3 08 48 PM

  1. I left one open comment. please update it before merging

Screen Shot 2022-10-03 at 3 10 44 PM

Makefile Outdated Show resolved Hide resolved
@okdas okdas requested a review from Olshansk October 6, 2022 00:05
@okdas okdas merged commit 0053199 into main Oct 6, 2022
@okdas okdas deleted the dk-add-linters branch October 6, 2022 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling tooling to support development, testing et al
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Automation] Automate Go Formatting and similar tasks
3 participants