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

Make trust_dns_proto::rr::Record serializable #1536

Merged
merged 5 commits into from
Aug 26, 2021

Conversation

mvforell
Copy link
Contributor

@mvforell mvforell commented Aug 11, 2021

When using the serde-config feature, I noticed that the trust_dns_proto::rr::Record is not Deserialize and Serialize. This can be achieved by adding a conditional derive to all its subtypes.

There are some deprecation warnings for crates/proto/src/rr/dnssec/rdata/key.rs, but I'm not sure how to fix them.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Everything looks good, I think changing the url serde option is the only thing to change.

crates/proto/Cargo.toml Outdated Show resolved Hide resolved
@bluejekyll
Copy link
Member

I see the deprecation errors... we'll need to think about how to resolve that.

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #1536 (6024eaf) into main (7733430) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1536   +/-   ##
=======================================
  Coverage   83.37%   83.37%           
=======================================
  Files         171      171           
  Lines       16936    16936           
=======================================
  Hits        14120    14120           
  Misses       2816     2816           

@mvforell
Copy link
Contributor Author

I see the deprecation errors... we'll need to think about how to resolve that.

Inspired by rust-lang/rust#87454, it would be possible to do this in proto/src/rr/dnssec/rdata/mod.rs:

pub mod dnskey;
pub mod ds;
#[allow(deprecated)]
pub mod key;
pub mod nsec;
// ...

Then we could remove the other allow[deprecated] that are currently present in proto/src/rr/dnssec/rdata/key.rs.

What do you think about this?

@bluejekyll
Copy link
Member

I just put up a PR for fixing the openssl dep on windows: #1543

@bluejekyll
Copy link
Member

ok, all tests are passing. Thanks for the PR!

@bluejekyll bluejekyll merged commit 3d0667a into hickory-dns:main Aug 26, 2021
@mvforell
Copy link
Contributor Author

You're welcome, thanks for creating and maintaining this project :)

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.

None yet

2 participants