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

Update to RFC 9562 #117

Merged
merged 11 commits into from May 12, 2024
Merged

Update to RFC 9562 #117

merged 11 commits into from May 12, 2024

Conversation

kohenkatz
Copy link
Contributor

@kohenkatz kohenkatz commented Feb 15, 2023

On 2022-10-17, the IETF uuidrev workgroup published a new draft that replaces peabody draft 04. On 2023-11-06, they released draft 14. On 2024-05-07, the IETF published RFC 9562, obsoleting RFC 4122.

This PR updates this project to implement the newest RFC. (fixes #118)

There are a few changes in documentation: mostly pointing to the new RFC URL, and adding comments as appropriate to explain the changes.

A new constant VariantRFC9562 is added, with the old VariantRFC4122 kept as an alias to it for backward compatibility.

The one major code change is in the implementation of UUID version 6. Since draft-01, this version now SHOULD use new pseudo-random data for the final 62 bytes, instead of using a clock_seq value for 14 of those bytes. (Although implementations MAY keep the old clock_seq behavior, this is discouraged, and is only allowed alongside keeping the MAC address as the node value. Since we are not keeping the MAC address, we also should not keep the clock_seq.

@kohenkatz kohenkatz marked this pull request as ready for review March 13, 2024 03:23
@kohenkatz
Copy link
Contributor Author

Sorry for the 13-month delay on this! I think it is ready for review now.

@kohenkatz kohenkatz changed the title Switch to newest IETF draft WIP: Switch to RFC9562 May 10, 2024
@kohenkatz kohenkatz marked this pull request as draft May 10, 2024 11:46
@kohenkatz kohenkatz marked this pull request as ready for review May 10, 2024 14:57
Copy link
Contributor Author

@kohenkatz kohenkatz left a comment

Choose a reason for hiding this comment

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

I have updated this to match the published RFC, which is number 9562.

As far as I can tell, the only changes since the 14th draft have been grammatical, not technical.

@kohenkatz kohenkatz changed the title WIP: Switch to RFC9562 Update to RFC 9562 May 10, 2024
@cameracker
Copy link
Contributor

I just realized the coverage isnt running anymore...

@cameracker
Copy link
Contributor

Got codecov set back up but it probably wont run on this until we merge. I added @dylan-bourque to the review to see if he has any feedback, but the changes seem correct to me and consistent with the verbiage in the latest RFC. Thank you for taking the time to update this.

@dylan-bourque
Copy link
Member

I gave this a quick scan and didn't see anything. I'm traveling today so won't have a chance to do a deeper review probably until Monday.

Copy link
Member

@dylan-bourque dylan-bourque left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of nits

func (g *Gen) NewV6() (UUID, error) {
/* https://datatracker.ietf.org/doc/html/rfc9562#name-uuid-version-6
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe include the source link in the godoc comment so that someone doesn't have to dive into the code to find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dylan-bourque I thought about that, but I think it's too much information that most users won't care about. The link is only there to show the origin of the bit layout below it, which is not something that needs to be in the godoc. The README has links to the full RFC, and that shows up in the godoc already.

generator.go Outdated Show resolved Hide resolved
func (g *Gen) NewV7() (UUID, error) {
var u UUID
/* https://www.ietf.org/archive/id/draft-peabody-dispatch-new-uuid-format-04.html#name-uuid-version-7
0 1 2 3
/* https://datatracker.ietf.org/doc/html/rfc9562#name-uuid-version-7
Copy link
Member

Choose a reason for hiding this comment

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

Same nit here

Co-authored-by: Cameron Ackerman <cameron_ackerman@selinc.com>
Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (22c52c2) to head (a3cbee1).
Report is 2 commits behind head on master.

❗ Current head a3cbee1 differs from pull request most recent head e2280ed. Consider uploading reports for the commit e2280ed to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #117   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          513       498   -15     
=========================================
- Hits           513       498   -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cameracker cameracker merged commit 0bd0b33 into gofrs:master May 12, 2024
2 checks passed
@kohenkatz kohenkatz deleted the update-draft-name branch May 12, 2024 14:40
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.

Link to new UUID draft
3 participants