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

use domain-name library to produce domain names from ip addresses #88

Merged
merged 4 commits into from
Jul 9, 2019

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jul 8, 2019

fixes #53

@dinosaure
Copy link
Member

dinosaure commented Jul 8, 2019

I'm fine with this, but I'm worried by this update:

 -     "e.0.0.2.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.0.9.0.0.4.0.5.4.1.0.0.a.2.ip6.arpa."
 +     "e.0.0.2.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.0.9.0.0.4.0.5.4.1.0.0.a.2.ip6.arpa"

in tests. So I don't know about internals of domain-name and believe that it's work perfectly but it's a breaking change or not?

@hannesm
Copy link
Member Author

hannesm commented Jul 8, 2019

(I just added a of_domain_name in here as well).

@dinosaure this is a breaking API change (though I couldn't find any users of to_domain_name)! mainly because the result are different now, it used to be: to_domain_name : t -> string list, now it is to_domain_name : t -> [`host] Domain_name.t.

If you want your trailing . back, you can use https://hannesm.github.io/domain-name/doc/domain-name/Domain_name/index.html#val-to_string with ~trailing:true.

this also superseeds #54 if accepted (which contains some discussion about trailing dots or not)

@avsm
Copy link
Member

avsm commented Jul 8, 2019

This lgtm; I can release with a major number bump to reflect the API change.

@hannesm would you be willing to also move domain-name into the mirage/ org so that we have all the dependencies in one place?

@dinosaure
Copy link
Member

@avsm can we integrate to this major release #84 ?

@hannesm
Copy link
Member Author

hannesm commented Jul 8, 2019

@avsm before doing a release, I'd prefer to clean up #35 / #37 -- val to_bytes : t -> string is just confusing every time i need to use it.

to have all the dependencies in one place

I'm not sure I follow, does this mean to move sexplib and astring and fmt also to the mirage org?

I'm fine with moving domain-name to mirage, but would prefer to have some guidelines about maintainership before doing this (see mirage/ocaml-dns#163 mirage/mirage-www#562), I'm a bit worried about increasing the number of repositories in the mirage organisation without having maintainers explicitly.

the other reason to not move it is due to travis CI which has n runners per organisation/user.

@hannesm
Copy link
Member Author

hannesm commented Jul 8, 2019

@dinosaure did you mean #87?

@dinosaure
Copy link
Member

dinosaure commented Jul 8, 2019

Ah yes sorry #87 👍

@avsm
Copy link
Member

avsm commented Jul 8, 2019

Ok, let's separate the repo move of domain-name discussion for elsewhere; it's not material to this particular PR if it's not an immediate change.

#87 can go in the major bump release as well indeed.

@avsm avsm merged commit 0d8a5a3 into mirage:master Jul 9, 2019
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.

Ipaddr.to_domain_name returns an empty label
4 participants