Skip to content

Conversation

@Manav-Aggarwal
Copy link
Member

@Manav-Aggarwal Manav-Aggarwal commented May 3, 2023

Resolves #911, resolves #919, resolves #910

@codecov
Copy link

codecov bot commented May 3, 2023

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 Files Coverage Δ
p2p/client.go 60.49% <ø> (+1.23%) ⬆️
p2p/gossip.go 75.92% <42.85%> (-6.08%) ⬇️
libs/testfactory/txs.go 72.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-Aggarwal Manav-Aggarwal self-assigned this May 3, 2023
@Manav-Aggarwal Manav-Aggarwal changed the title add //nolint:gosec where math/rand is used Resolve CI/CD linter issue with rand library May 3, 2023
@Manav-Aggarwal Manav-Aggarwal marked this pull request as ready for review May 3, 2023 06:58
@smuu
Copy link
Contributor

smuu commented May 3, 2023

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 nashqueue force-pushed the manav/fix_linter_rand branch from 9b1272f to b156632 Compare May 3, 2023 09:24
@Manav-Aggarwal Manav-Aggarwal changed the title Resolve CI/CD linter issue with rand library Disable CI/CD linter issue with rand library and fix data race May 5, 2023
@Manav-Aggarwal Manav-Aggarwal changed the title Disable CI/CD linter issue with rand library and fix data race Disable CI/CD linter check with rand library and fix data race May 5, 2023
@Manav-Aggarwal
Copy link
Member Author

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-Aggarwal Manav-Aggarwal changed the title Disable CI/CD linter check with rand library and fix data race Disable CI/CD linter check with rand library and fix server overlap May 5, 2023
@Manav-Aggarwal Manav-Aggarwal linked an issue May 5, 2023 that may be closed by this pull request
@Manav-Aggarwal Manav-Aggarwal requested review from S1nus, smuu and tuxcanfly May 5, 2023 05:46
@Manav-Aggarwal Manav-Aggarwal changed the title Disable CI/CD linter check with rand library and fix server overlap fix: Disable CI/CD linter check with rand library and fix server overlap May 5, 2023
@Manav-Aggarwal Manav-Aggarwal added T:bug Something isn't working T:testing Related to testing labels May 5, 2023
@Manav-Aggarwal Manav-Aggarwal changed the title fix: Disable CI/CD linter check with rand library and fix server overlap fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issues May 5, 2023
@Manav-Aggarwal Manav-Aggarwal changed the title fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issues fix: Disable CI/CD linter check with rand library, fix server overlap, and data race issue May 5, 2023
smuu
smuu previously approved these changes May 5, 2023
Copy link
Contributor

@smuu smuu left a comment

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 review May 5, 2023 13:43

Race test fail

nashqueue
nashqueue previously approved these changes May 5, 2023
@Manav-Aggarwal Manav-Aggarwal merged commit d7f6772 into main May 5, 2023
@Manav-Aggarwal Manav-Aggarwal deleted the manav/fix_linter_rand branch May 5, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T:bug Something isn't working T:testing Related to testing

Projects

None yet

5 participants