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

Use crypto/rand for random id generation. #1044

Merged
merged 3 commits into from Dec 11, 2019
Merged

Use crypto/rand for random id generation. #1044

merged 3 commits into from Dec 11, 2019

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 5, 2019

Notably, if crypto/rand returns an error (very unlikely) this will panic.

Fixes #1043 and #1037

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 5, 2019

Codecov Report

Merging #1044 into master will decrease coverage by 0.07%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
- Coverage   55.05%   54.98%   -0.08%     
==========================================
  Files          41       41              
  Lines        9879     9872       -7     
==========================================
- Hits         5439     5428      -11     
- Misses       3419     3422       +3     
- Partials     1021     1022       +1
Impacted Files Coverage Δ
msg.go 76.55% <60%> (-0.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22cda6d...a509d69. Read the comment docs.

miekg
miekg approved these changes Dec 6, 2019
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 7, 2019

crypto/rand basically never returns an error, unless someone set up a super-strict sandbox, in which case they probably have opinions that can be encoded as a custom Id function. I'd just go ahead and make it a panic, that's what I did in the version in the Google monorepo.

Copy link
Collaborator

@tmthrgd tmthrgd left a comment

I like this approach and it's a nice simplification. Just a few minor nits.

msg.go Outdated Show resolved Hide resolved
msg.go Outdated Show resolved Hide resolved
msg.go Show resolved Hide resolved
msg.go Outdated Show resolved Hide resolved
@miekg
Copy link
Owner

@miekg miekg commented Dec 9, 2019

@yongtang
Copy link

@yongtang yongtang commented Dec 10, 2019

Looks like all review comments have been covered, @miekg @tmthrgd can you check to see if this PR is ready to go?

@miekg
Copy link
Owner

@miekg miekg commented Dec 11, 2019

@miekg
Copy link
Owner

@miekg miekg commented Dec 11, 2019

merging and doing a small follow up PR; this allows me to tag a release where this at least is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants