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

Expose the reference UUID generator constructor #42

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

theckman
Copy link
Member

This change exposes the reference UUID generator in the package, so that
consumers can create a new instance and make use of it. This is being done so
that consumers can provide their own Hardware Address generation function, in
case they want to randomize the MAC address as to not expose it.

This works by now exposing the rfc4122Generator type as Gen, and adding
constructor functions that scaffold the generator appropriately. The tests were
also enhanced to confirm that the HWAddrFunc we provide is indeed being
invoked.

Fixes #41

Signed-off-by: Tim Heckman t@heckman.io

@theckman theckman added this to the 3.0.0 milestone Jul 28, 2018
@theckman theckman requested a review from zerkms July 28, 2018 19:12
generator.go Outdated
// address for all the future UUIDs generated by it. If you'd like to switch the
// MAC address being used, you'll need to create a new generator using this
// function.
func NewGenHWAF(hwaf HWAddrFunc) *Gen {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll be the first to admit that I'm not sold on this function name. Couldn't think of anything better in the moment, though.

Copy link
Member

Choose a reason for hiding this comment

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

I personally tend to name things with With in such cases: NewGenWithHWAF

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds better to me.

@coveralls
Copy link

coveralls commented Jul 28, 2018

Pull Request Test Coverage Report for Build 127

  • 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 99.115%

Totals Coverage Status
Change from base Build 123: 0.008%
Covered Lines: 336
Relevant Lines: 339

💛 - Coveralls

This change exposes the reference UUID generator in the package, so that
consumers can create a new instance and make use of it. This is being done so
that consumers can provide their own Hardware Address generation function, in
case they want to randomize the MAC address as to not expose it.

This works by now exposing the `rfc4122Generator` type as `Gen`, and adding
constructor functions that scaffold the generator appropriately. The tests were
also enhanced to confirm that the `HWAddrFunc` we provide is indeed being
invoked.

Fixes #41

Signed-off-by: Tim Heckman <t@heckman.io>
@niaow
Copy link
Member

niaow commented Jul 29, 2018

Are we sure we even want a hwAddrFunc at all? We could alternately modify NewV1 to accept a hardware address and add a global HWAddr func to get the system hardware address instead.

So existing calls could be replaced with:

NewV1(HWAddr())

and if the user wants an alternate address:

NewV1(addr)

@zerkms
Copy link
Member

zerkms commented Jul 29, 2018

@jadr2ddude having it as a factory function abstracts UUID client from its constructor: you as a client may generate new UUIDs and be isolated from making a decision on how and where from the hardware address is obtained from.

@theckman
Copy link
Member Author

theckman commented Jul 29, 2018

@jadr2ddude I am assuming less than 10% of users will make use of this functionality, and so I don't think it makes sense to require the 90% to consider their hardware address as part of the API they interact with. For the majority we should silently do what they expect. We can then expose knobs that allow the minority to have the control they want.

@theckman theckman merged commit 97e53ed into master Aug 2, 2018
theckman added a commit that referenced this pull request Aug 2, 2018
Expose the reference UUID generator constructor

Signed-off-by: Tim Heckman <t@heckman.io>
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.

4 participants