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

add ZONEMD support #229

Merged
merged 1 commit into from
Sep 29, 2023
Merged

add ZONEMD support #229

merged 1 commit into from
Sep 29, 2023

Conversation

xofyarg
Copy link
Contributor

@xofyarg xofyarg commented Sep 23, 2023

This commit add support for ZONEMD record type, described in RFC 8976.

Since root is rolling it out, there's no reason the standard is missing from the library :-p

Copy link
Member

@partim partim left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! It looks good with only a few minor comments.

/// hashing function.
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[repr(u8)]
Copy link
Member

Choose a reason for hiding this comment

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

Does that actually work? I would assume the compiler needs to be able to distinguish between the unassigned and private variant both of which can carry a full u8.

Copy link
Contributor Author

@xofyarg xofyarg Sep 26, 2023

Choose a reason for hiding this comment

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

It only works with field-less enums. Removed.

Edit: It is required because of https://github.com/rust-lang/rust/blob/master/compiler/rustc_error_codes/src/error_codes/E0732.md

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think it makes too much sense to add the value to Simple, given that you convert explicitly in the From<_> impls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. These are removed.

}
}

/// The Scheme is an 8-bit unsigned integer that identifies the
Copy link
Member

Choose a reason for hiding this comment

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

Could you start the docstring with a single one-line summary? This will then show in the index pages of the module and gives a hint as to what the type is for …

}
}

/// The Hash Algorithm is an 8-bit unsigned integer that identifies
Copy link
Member

Choose a reason for hiding this comment

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

One-line summary here as well.

@xofyarg xofyarg force-pushed the zonemd branch 2 times, most recently from bebab87 to 0cda505 Compare September 26, 2023 20:28
@xofyarg
Copy link
Contributor Author

xofyarg commented Sep 26, 2023

Thanks for the review, @partim. I've addressed all the comments, please take another look.

This commit add support for ZONEMD record type, described in RFC 8976.
@partim
Copy link
Member

partim commented Sep 29, 2023

Thank you for the changes! Looking good now.

Given that the record type enums are marked as non-exhaustive, I think this isn’t a breaking change and can be merged right away.

@partim partim merged commit 28b871f into NLnetLabs:main Sep 29, 2023
12 checks passed
@xofyarg xofyarg deleted the zonemd branch September 30, 2023 00:09
partim added a commit that referenced this pull request Oct 18, 2023
Breaking changes

* Move the `flatten_into` method for converting domain names into a
  straight, flat form into a new `FlattenInto` trait. This trait is only
  implemented for types that actually are or contain domain names. ([#216])
* Marked various methods and functions that return values without side
  effects as `#[must_use]`. ([#228] by [@WhyNotHugo])
* Changed the signature of `FoundSrvs::merge` to use a non-mut `other`.
  ([#232])

New

* Added support for the ZONEMD record type. ([#229] by [@xofyarg])
* Re-exported the _octseq_ crate as `dep::octseq`. ([#230])
* Added a blanket impl for mut refs to `Composer`. ([#231] by [@xofyarg])
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.

2 participants