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

[TECHDEBT][Shared] Address legacy Run golangci-lint errors #295

Closed
7 tasks
jessicadaugherty opened this issue Oct 6, 2022 · 6 comments · Fixed by #483
Closed
7 tasks

[TECHDEBT][Shared] Address legacy Run golangci-lint errors #295

jessicadaugherty opened this issue Oct 6, 2022 · 6 comments · Fixed by #483
Assignees
@h5law
Labels
code health Nice to have code improvement community Open to or owned by a non-core team member core starter task Good for newcomers, but aimed at core team members though still open for everyone tooling tooling to support development, testing et al
Milestone
M*: Pocket QoL (Q...

Comments

@jessicadaugherty
Copy link
Contributor

jessicadaugherty commented Oct 6, 2022 •
edited
Loading

Objective

Improve unit tests by addressing legacy linting errors

Origin Document

Raised by review of #235 (comment)

Screen_Shot_2022-10-06_at_10 50 28_AM

Goals

  • Block PRs that break unit tests
  • Block PRs that break linting errors
  • Display code changes that do not follow style guidelines

Deliverables

  • A PR that tends to all of the legacy linting errors in the repo
  • A PR that updates the linting github workflow to block (rather than simply display a warning) on golang and repo linting errors

Non-goals / Non-deliverables

  • Changing any business logic in the codebase

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid

Creator: @jessicadaugherty
Co-Owners: @Olshansk
@jessicadaugherty jessicadaugherty changed the title [TECHDEBT][Shared] Create Run golangci-lint error [TECHDEBT][Shared] Create Run golangci-lint error warning Oct 6, 2022
@Olshansk Olshansk added the tooling tooling to support development, testing et al label Oct 6, 2022
@jessicadaugherty jessicadaugherty changed the title [TECHDEBT][Shared] Create Run golangci-lint error warning [TECHDEBT][Shared] Address legacy Run golangci-lint errors Oct 6, 2022
@Olshansk
Copy link
Member

@jessicadaugherty Can we include this in iteration 4? It's a lower priority, but it blocks us from enabling the autolinters

@Olshansk Olshansk added the core starter task Good for newcomers, but aimed at core team members though still open for everyone label Dec 15, 2022
@Olshansk Olshansk self-assigned this Dec 15, 2022
@deblasis deblasis mentioned this issue Dec 27, 2022
Merged
16 tasks
@Olshansk Olshansk removed their assignment Jan 3, 2023
@jessicadaugherty jessicadaugherty added the community Open to or owned by a non-core team member label Jan 31, 2023
@h5law
Copy link
Contributor

h5law commented Jan 31, 2023 •
edited
Loading

@jessicadaugherty I'll do this (I don't know when iteration 4 is but can have it done this week). I find these errors annoying anyway so it'll be nice to get rid of em!

Just to confirm - fix the errors from the golanglint-ci test throughout the codebase and make the workflow blocking on failure?

@jessicadaugherty
Copy link
Contributor Author

Amazing, thanks @h5law! I assigned this out to you on DeWork. LOL iteration 4 was in like October or something so this was originally a priority backlog item that kept getting refactored. Agreed it would improve contributor QoL! Appreciate it.

Just to confirm - fix the errors from the golanglint-ci test throughout the codebase and make the workflow blocking on failure?

Exactly! I'd take a look at the linked PR in "Origin Document" for more context on where we stumbled on this need. And, of course, lmk if you have any questions!

@okdas
Copy link
Member

okdas commented Jan 31, 2023 •
edited
Loading

Hey @h5law, thank you for looking into this!

Here is a related thread for the existing "Cannot open: File exists" errors : golangci/golangci-lint-action#23

Basically, from what I understand, the packages are getting downloaded by golangci-lint-action are already present because of golang cache. Or due to the fact we need to generate mocks and all that stuff that requires us to download the packages prior to executing golangci-lint-action. Lmk if you have any questions — I've looked into that briefly a couple of months ago but never committed a change.

@Olshansk
Copy link
Member

Just wanted to provide some context to what @okdas mentioned.

@h5law is working on enabling the golang CI linter by tending to ALL OF the issues it raises throughout the codebase.

One of the many such issues is the fact that there is a ❌ when the test_results.json is being uploaded. The following screenshot is taken from this link

Screenshot 2023-01-31 at 9 06 38 AM

The important to note is that this is a false positive. The file gets uploaded and there is no real issue. However, seeing a red X can cause confusion and lead to developers spending time looking in a place where there is no problem.

The information Dima provided is a potential starting point to understand how/if we could resolve this as well.

@h5law h5law mentioned this issue Feb 2, 2023
Merged
16 tasks
@h5law h5law closed this as completed in #483 Feb 6, 2023
h5law added a commit that referenced this issue Feb 6, 2023
@h5law
[TECHDEBT][Shared] Address legacy Run golangci-lint errors (Issue #295…
ceb8b9b 
…) (#483)

## Description

This PR addresses all the errors from running `golangci-lint run`. The
errors which were genuine have been addressed and those that are `false
positives` have been ignored with the relevant `//nolint:{linter}`
comments with their required justifications.

The `golangci-lint` github worklow has also been separated into a
separate job and file with its own setup process which should counteract
the `Cannot open file: File exists` errors

## Issue

Fixes #295 
## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Fix legacy issues from linter
- Add comments to ignore false positives
- Seperate `golanci-lint` into its own worklow

## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`


## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist
- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@github-project-automation github-project-automation bot moved this from Backlog to Done in V1 Dashboard Feb 6, 2023
@jessicadaugherty
Copy link
Contributor Author

@bryanchriswhite is this helpful for documenting PR review standards like we discussed in the retro?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@h5law h5law

Labels
code health Nice to have code improvement community Open to or owned by a non-core team member core starter task Good for newcomers, but aimed at core team members though still open for everyone tooling tooling to support development, testing et al
Projects
V1 Dashboard
Status: Done
Milestone
M*: Pocket QoL (Quality of Life)
Development

Successfully merging a pull request may close this issue.

[TECHDEBT][Shared] Address legacy Run golangci-lint errors (Issue #295) pokt-network/pocket
4 participants
@Olshansk @okdas @h5law @jessicadaugherty