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

moving random.go from utils #11017

Merged
merged 1 commit into from Feb 27, 2015
Merged

moving random.go from utils #11017

merged 1 commit into from Feb 27, 2015

Conversation

brahmaroutu
Copy link
Contributor

Addresses #10962
Signed-off-by: Srini Brahmaroutu srbrahma@us.ibm.com

@brahmaroutu
Copy link
Contributor Author

@crosbymichael I have moved random into pkg/common as I feel the code belongs there. Please let me know if you would want to refactor differently.

ahmetb
ahmetb reviewed Feb 26, 2015
View reviewed changes
pkg/common/randomid_test.go
}
}

func TestGenerateID(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be TestGenerateRandomID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmetalpbalkan Fixed.

@ahmetb
Copy link
Contributor

ahmetb commented Feb 26, 2015

This package is turning into a randomutils and probably should initialize its own instance of rand.Random in an init function seeded with time. Otherwise it's going to generate same IDs over and over again. Also it's better if this does not rely on the pkg math/rand's globalRandom instance.

@brahmaroutu
moving random.go from utils
53ece33 
Closes moby#10962
Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>
@brahmaroutu
Copy link
Contributor Author

I am not sure if we should address the concern here. Please keep the scope of this to moving out of Utils. We can open a new issue on how Random generation of IDs work.

@ahmetb
Copy link
Contributor

ahmetb commented Feb 26, 2015

@brahmaroutu You're right it's out of scope for this PR. Thanks.

LGTM (not a maintainer)

@estesp
Copy link
Contributor

estesp commented Feb 26, 2015

LGTM

estesp added a commit that referenced this pull request Feb 27, 2015
@estesp
Merge pull request #11017 from brahmaroutu/random_10962
f5850e8 
moving random.go from utils
@estesp estesp merged commit f5850e8 into moby:master Feb 27, 2015
@brahmaroutu brahmaroutu deleted the random_10962 branch March 6, 2015 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brahmaroutu @ahmetb @estesp @dmp42 @jessfraz @GordonTheTurtle