Fix for --sourceaddr bad behaviour #110

vlevigneron · 2019-07-20T21:54:45Z

fixes #38
danish and swedish translation missing

matsduf

In sv.po add:

msgid "Address '"
msgstr "IP-adress '"

msgid "' cannot be used as source address for DNS queries."
msgstr "' kan inte användas som källadress för DNS-frågor."

In da.po just copy the English version as placeholder.

mattias-p

The code looks nice, but it would be great if you could take another look at dependencies, translatable strings and indentation. I've provided comments.

lib/Zonemaster/CLI.pm

mattias-p · 2019-08-05T07:25:22Z

Makefile.PL

@@ -21,6 +21,7 @@ build_requires( 'Test::More' => 0, );
 requires(
    'JSON::XS'           => 0,
    'Locale::TextDomain' => 1.23,
+    'Net::Interface'     => 0,


I can't find a binary package for this on CentOS. Have you checked for alternatives?

@mattias-p I only found Net::Interface that can handle with IPv4 and IPv6. IO::Interface for example does not work with IPv6. Any advices for an alternate library ?

At least for CentOS 7 I find a package: https://centos.pkgs.org/7/openfusion-x86_64/perl-Net-Interface-1.012-2.of.el7.x86_64.rpm.html

According to https://rt.cpan.org/Public/Bug/Display.html?id=124582, me should add this somewhere in travis.yml
mkdir /usr/include/sys/; ln -s /usr/include/bits/socket.h /usr/include/sys/socket.h to prevent the fail on Travis.

@vlevigneron Thanks for the clarification regarding the choice of library. And no, I don't have any alternatives up my sleeve.

lib/Zonemaster/CLI.pm

matsduf · 2019-10-02T12:29:10Z

@vlevigneron, what is the milestone of this PR? v2019.2 or v2019.3?

… share/fr.po share/sv.po ( share/en.po removed ). Merge remote-tracking branch 'upstream/develop' into fix-issue-zonemaster-cli-38

mattias-p · 2021-01-12T11:31:27Z

Should the installation instruction be updated with the new dependency in this PR as well, or do we do that as part of installation testing?

vlevigneron · 2021-01-12T11:55:06Z

@mattias-p Good point. I guess that it is usually done during packaging process, can anyone confirm ? If this is not the case, it can be modified in this PR.
@Matts I finally fixed the issue I had with Travis, it has passed all the checks. Please approve again.

matsduf · 2021-01-12T12:05:04Z

@mattias-p Good point. I guess that it is usually done during packaging process, can anyone confirm ? If this is not the case, it can be modified in this PR.

If possible it should be done now. For FreeBSD it is p5-Net-Interface to be installed. CentOS has already CPAN dependencies so I guess it is OK to add it there. Fore Debian/Ubuntu we should have binary package.

@Matts I finally fixed the issue I had with Travis, it has passed all the checks. Please approve again.

Wrong "(a)Matts". I will review again.

Matts · 2021-01-12T12:31:12Z

You are not the first, and most certainly will not be the last that accidentally mentions me :P

My own fault for having a common name lmao. Besides, it is interesting to have a look at what other people are up to

matsduf · 2021-01-12T13:33:43Z

You are not the first, and most certainly will not be the last that accidentally mentions me :P

My own fault for having a common name lmao. Besides, it is interesting to have a look at what other people are up to

@Matts, you were early to get a short user name. :-) You are welcome to Zonemaster!

mattias-p

Everything looks good to me, except two problems I should have spotted earlier. Sorry about that.

mattias-p · 2021-01-12T14:05:37Z

share/da.po

+#, perl-brace-format
+msgid "Address {address} cannot be used as source address for DNS queries.\n"
+msgstr "Address {address} cannot be used as source address for DNS queries.\n"
+


I'm sorry I didn't spot this earlier, but I don't think this should be included. It only makes it difficult for the Danish translators to notice that there is something new for them to translate.

mattias-p · 2021-01-12T14:06:17Z

share/nb.po

+#, perl-brace-format
+msgid "Address {address} cannot be used as source address for DNS queries.\n"
+msgstr "Address {address} cannot be used as source address for DNS queries.\n"
+


This should also be excluded for the same reason.

@matsduf As you asked for something different (but it was a long time ago), do you think that @mattias-p proposal is reasonable ?

That is fine. I am not sure if it should be empty or non-existent, but at least the latter will mean that msgid will shown, and then when you translate you will be prompted.

vlevigneron added 2 commits July 20, 2019 23:46

- Fixes zonemaster#38

59aff45

- Add dependency.

69d3409

vlevigneron requested review from matsduf and mattias-p July 20, 2019 21:55

- Message modified for translation.

b8646f9

matsduf requested changes Jul 26, 2019

View reviewed changes

matsduf mentioned this pull request Jul 26, 2019

Less helpful error message when source address of --sourceaddr cannot be used #38

Closed

- Add swedish translation and danish placeholders.

5378498

matsduf approved these changes Aug 1, 2019

View reviewed changes

mattias-p requested changes Aug 5, 2019

View reviewed changes

matsduf added this to the v2019.3 milestone Oct 31, 2019

matsduf modified the milestones: v2019.3, v2020.1 Nov 20, 2019

matsduf modified the milestones: v2020.1, v2020.2 Sep 14, 2020

matsduf added the P-High Priority: Issue to be solved before other label Nov 12, 2020

vlevigneron added 5 commits January 12, 2021 00:25

Merged and resolved the conflict in lib/Zonemaster/CLI.pm share/da.po…

804acf9

… share/fr.po share/sv.po ( share/en.po removed ). Merge remote-tracking branch 'upstream/develop' into fix-issue-zonemaster-cli-38

- Change string for translation purposes.

bef81ce

- Missing newline.

0c2e9b4

- Fix for Net::Interface in Travis (should work).

2f57ac7

- Fix permission issue in Travis

0709f13

matsduf previously approved these changes Jan 12, 2021

View reviewed changes

Another try to fix.

c3b970a

vlevigneron dismissed matsduf’s stale review via c3b970a January 12, 2021 11:23

Typo

09a5fc4

- Installation instructions updated.

c1705d5

matsduf previously approved these changes Jan 12, 2021

View reviewed changes

mattias-p reviewed Jan 12, 2021

View reviewed changes

- Remove english messages from translation files (da.po/nb.po).

f01f486

vlevigneron dismissed matsduf’s stale review via f01f486 January 12, 2021 16:14

matsduf approved these changes Jan 12, 2021

View reviewed changes

mattias-p approved these changes Jan 12, 2021

View reviewed changes

vlevigneron merged commit beae6f0 into zonemaster:develop Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for --sourceaddr bad behaviour #110

Fix for --sourceaddr bad behaviour #110

vlevigneron commented Jul 20, 2019 •

edited

Loading

matsduf left a comment

mattias-p left a comment

mattias-p Aug 5, 2019

vlevigneron Jan 12, 2021

matsduf Jan 12, 2021

vlevigneron Jan 12, 2021

mattias-p Jan 12, 2021

matsduf commented Oct 2, 2019

mattias-p commented Jan 12, 2021

vlevigneron commented Jan 12, 2021

matsduf commented Jan 12, 2021

Matts commented Jan 12, 2021 •

edited

Loading

matsduf commented Jan 12, 2021

mattias-p left a comment

mattias-p Jan 12, 2021

mattias-p Jan 12, 2021

vlevigneron Jan 12, 2021

matsduf Jan 12, 2021

Fix for --sourceaddr bad behaviour #110

Fix for --sourceaddr bad behaviour #110

Conversation

vlevigneron commented Jul 20, 2019 • edited Loading

matsduf left a comment

Choose a reason for hiding this comment

mattias-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Oct 2, 2019

mattias-p commented Jan 12, 2021

vlevigneron commented Jan 12, 2021

matsduf commented Jan 12, 2021

Matts commented Jan 12, 2021 • edited Loading

matsduf commented Jan 12, 2021

mattias-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlevigneron commented Jul 20, 2019 •

edited

Loading

Matts commented Jan 12, 2021 •

edited

Loading