Skip to content

fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issue - #916

Merged
Manav-Aggarwal merged 7 commits into
mainfrom
manav/fix_linter_rand
May 5, 2023
Merged

fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issue#916
Manav-Aggarwal merged 7 commits into
mainfrom
manav/fix_linter_rand

Conversation

@Manav-Aggarwal

@Manav-AggarwalManav-Aggarwal commented May 3, 2023

Copy link
Copy Markdown
Member

Resolves#911, resolves#919, resolves#910

@codecov

codecovBot commented May 3, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 55.55% and project coverage change: -0.28⚠️

Comparison is base (ee5582f) 56.11% compared to head (4b6ef57) 55.84%.

Additional details and impacted files
@@ Coverage Diff @@## main #916 +/- ##
==========================================
- Coverage 56.11% 55.84% -0.28% 
==========================================
Files 66 66 Lines 10705 10709 +4 ==========================================
- Hits 6007 5980 -27 - Misses 3830 3858 +28 - Partials 868 871 +3 
Impacted FilesCoverage Δ
p2p/client.go60.49% <ø> (+1.23%)⬆️
p2p/gossip.go75.92% <42.85%> (-6.08%)⬇️
libs/testfactory/txs.go72.22% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@Manav-AggarwalManav-Aggarwal self-assigned this May 3, 2023
@Manav-AggarwalManav-Aggarwal changed the title add //nolint:gosec where math/rand is usedResolve CI/CD linter issue with rand libraryMay 3, 2023
@Manav-Aggarwal
Manav-Aggarwal marked this pull request as ready for review May 3, 2023 06:58
@smuu

smuu commented May 3, 2023

Copy link
Copy Markdown
Contributor

In my opinion, the PR title and later the commit message should not contain "Resolve" because this PR does not resolve the security warning, it just disables the check.
I'm okay with not fixing it because it's used in the tests. But this should not be used in production code.

@nashqueue
nashqueueforce-pushed the manav/fix_linter_rand branch from 9b1272f to b156632CompareMay 3, 2023 09:24
@Manav-AggarwalManav-Aggarwal changed the title Resolve CI/CD linter issue with rand libraryDisable CI/CD linter issue with rand library and fix data raceMay 5, 2023
@Manav-AggarwalManav-Aggarwal changed the title Disable CI/CD linter issue with rand library and fix data raceDisable CI/CD linter check with rand library and fix data raceMay 5, 2023
@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

In my opinion, the PR title and later the commit message should not contain "Resolve" because this PR does not resolve the security warning, it just disables the check. I'm okay with not fixing it because it's used in the tests. But this should not be used in production code.

Since the math/rand library is used across multiple files, there's not just one place where we can put a warning. Where do we think is it most appropriate to document the math/rand warning if we don't want it to be used in production?

@Manav-AggarwalManav-Aggarwal changed the title Disable CI/CD linter check with rand library and fix data raceDisable CI/CD linter check with rand library and fix server overlapMay 5, 2023
@Manav-AggarwalManav-Aggarwal linked an issue May 5, 2023 that may be closed by this pull request
@Manav-AggarwalManav-Aggarwal changed the title Disable CI/CD linter check with rand library and fix server overlapfix: Disable CI/CD linter check with rand library and fix server overlapMay 5, 2023
@Manav-AggarwalManav-Aggarwal added T:bug Something isn't working T:testing Related to testing labels May 5, 2023
@Manav-AggarwalManav-Aggarwal changed the title fix: Disable CI/CD linter check with rand library and fix server overlapfix: Disable CI/CD linter check with rand library, fix server overlap, and data race issuesMay 5, 2023
@Manav-AggarwalManav-Aggarwal changed the title fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issuesfix: Disable CI/CD linter check with rand library, fix server overlap, and data race issueMay 5, 2023
smuu
smuu previously approved these changes May 5, 2023

@smuusmuu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

nashqueue
nashqueue previously approved these changes May 5, 2023
smuu
smuu previously approved these changes May 5, 2023
@nashqueue
nashqueue dismissed their stale reviewMay 5, 2023 13:43

Race test fail

Comment threadp2p/client.go Outdated
nashqueue
nashqueue previously approved these changes May 5, 2023
@Manav-Aggarwal
Manav-Aggarwal merged commit d7f6772 into mainMay 5, 2023
@Manav-Aggarwal
Manav-Aggarwal deleted the manav/fix_linter_rand branch May 5, 2023 17:19
@nashqueuenashqueue mentioned this pull request May 15, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T:bugSomething isn't workingT:testingRelated to testing

Projects

None yet

4 participants

@Manav-Aggarwal@smuu@gupadhyaya@nashqueue