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

The +nocl is supported much wider than +noclass #51429

Merged
merged 4 commits into from Feb 6, 2019

Conversation

Projects
None yet
2 participants
@yosnoop
Copy link
Contributor

commented Jan 30, 2019

They are same options.

in bin/dig/dig.c
...
case 'l': /* class */
    /* keep +cl for backwards compatibility */
    FULLCHECK2("cl", "class");
    lookup->noclass = !state;
    break;

Seems the +nocl is introduced with tag v9_0_1 while +noclass is with tag v9_11_0.
(the tag could be different with distribution's version)

$ cat /etc/redhat-release 
Red Hat Enterprise Linux Server release 7.6 (Maipo)

$ dig -v
DiG 9.9.4-RedHat-9.9.4-73.el7_6

$ dig +nocl +short -t soa saltstack.com
ns-39.awsdns-04.com. operations.saltstack.com. 1 7200 900 1209600 500

$ dig +noclass +short -t soa saltstack.com
Invalid option: +noclass
...

What does this PR do?

This PR will replace +noclass option for dig with +nocl in order to support more OS without compromising anything.

What issues does this PR fix or reference?

#51428

Previous Behavior

$ cat lookup.py 
import salt.utils

def run():
    dig = salt.utils.dns.lookup('google.com', 'soa', method='dig')
    host = salt.utils.dns.lookup('google.com', 'soa', method='host')
    return { "dig": dig, "host": host }
$ 
$ sudo salt-call lookup.run                                                                                                                                   
[WARNING ] dig returned (1): Invalid option: +noclass
Usage:  dig [@global-server] [domain] [q-type] [q-class] {q-opt}
            {global-d-opt} host [@local-server] {local-d-opt}
            [ host [@local-server] {local-d-opt} [...]]

Use "dig -h" (or "dig -h | more") for complete list of options
local:
    ----------
    dig:
        None
    host:
        - ns1.google.com. dns-admin.google.com. 231793091 900 900 1800 60

New Behavior

$ sudo salt-call lookup.run 
local:
    ----------
    dig:
        - ns1.google.com. dns-admin.google.com. 231793091 900 900 1800 60
    host:
        - ns1.google.com. dns-admin.google.com. 231793091 900 900 1800 60

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@yosnoop

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@dwoz Since we use mock for unit test, this has not been detected. Is it better to add a test would be able to detect this sort of issue?

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@yosnoop Yes, since this is a bugfix. Having a regression tests is ideal.

@yosnoop yosnoop force-pushed the yosnoop:replace-noclass-with-nocl-for-dig branch 3 times, most recently from 7afe4c2 to 89a0d6f Feb 2, 2019

yosnoop added some commits Feb 3, 2019

The +nocl is supported much wider than +noclass
They are same options.
in bin/dig/dig.c
...
case 'l': /* class */
    /* keep +cl for backwards compatibility */
    FULLCHECK2("cl", "class");
    lookup->noclass = !state;
    break;

Seems the +nocl is introduced with tag v9_0_1 while +noclass is with tag
v9_11_0.
(the tag could be different with distribution's version)

$ dig -v
DiG 9.9.4-RedHat-9.9.4-73.el7_6

$ dig +nocl +short -t soa saltstack.com
ns-39.awsdns-04.com. operations.saltstack.com. 1 7200 900 1209600 500

$ dig +noclass +short -t soa saltstack.com
Invalid option: +noclass
...

@yosnoop yosnoop force-pushed the yosnoop:replace-noclass-with-nocl-for-dig branch from 89a0d6f to effc883 Feb 3, 2019

In case the test machine can't resolve
With -v option, dig does not attemp to resolve while verifying all the given
options.

@yosnoop yosnoop force-pushed the yosnoop:replace-noclass-with-nocl-for-dig branch from effc883 to 14e6fb7 Feb 3, 2019

@yosnoop

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@yosnoop Yes, since this is a bugfix. Having a regression tests is ideal.

@dwoz I pushed commits to implement a test for the bug. Could you take a look, please?

@dwoz

dwoz approved these changes Feb 6, 2019

@dwoz dwoz merged commit 1f981c6 into saltstack:2018.3.4 Feb 6, 2019

10 checks passed

WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details

@yosnoop yosnoop deleted the yosnoop:replace-noclass-with-nocl-for-dig branch Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.