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

Introduce server specification in nameserver_info module #168

Merged

Conversation

hakong
Copy link
Contributor

@hakong hakong commented Nov 27, 2023

SUMMARY

This merge request introduces the ability to specify custom DNS servers for resolving nameserver information in the nameserver_info module. It includes enhancements to the ResolveDirectlyFromNameServers class to handle custom server addresses and updates the nameserver_info module to accept a new 'servers' parameter.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

community.dns.nameserver_info

ADDITIONAL INFORMATION

Enhanced the ResolveDirectlyFromNameServers class to accept a new argument 'server_addresses'. This allows specifying custom DNS servers for resolution, overriding the default nameservers. Adjusted the constructor and relevant methods to handle the provided server addresses.
Modified the nameserver_info module to include a new 'servers' parameter, enabling users to specify custom DNS servers for nameserver resolution. Updated argument_spec in AnsibleModule initialization and adjusted resolver initialization to use the provided server addresses.
Copy link

github-actions bot commented Nov 27, 2023

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main:
https://ansible-collections.github.io/community.dns/branch/main

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b93cd64) 96.81% compared to head (91c7d9c) 96.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #168   +/-   ##
=======================================
  Coverage   96.81%   96.81%           
=======================================
  Files         107      107           
  Lines        7349     7349           
  Branches     1022     1022           
=======================================
  Hits         7115     7115           
  Misses        167      167           
  Partials       67       67           
Flag Coverage Δ
integration 40.70% <100.00%> (-0.04%) ⬇️
sanity 31.62% <50.00%> (ø)
units 96.07% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hakong hakong changed the title Nameserver info custom dns Add Optional Recursive Lookup for Authoritative Nameservers in ResolveDirectlyFromNameServers Nov 27, 2023
@hakong hakong changed the title Add Optional Recursive Lookup for Authoritative Nameservers in ResolveDirectlyFromNameServers Introduce server specification in nameserver_info module Nov 27, 2023
@hakong hakong marked this pull request as ready for review November 27, 2023 14:43
@hakong
Copy link
Contributor Author

hakong commented Nov 27, 2023

@felixfontein Does this look OK? I need is to be able to specify the nameserver to contact. I've tested this and it seems to work fine.
I'm not familiar with writing tests, sorry they aren't included. I'll might give it a try later this week.

  tasks:
    - name: Retrieve name servers of felix.fontein.de
      community.dns.nameserver_info:
        name: "felix.fontein.de"
        servers:
          - "208.67.222.222"
          - "208.67.220.220"
      register: __ff_ns

    - name: "Debug print __ff_ns"
      ansible.builtin.debug:
        msg: "{{ __ff_ns }}"
TASK [Retrieve name servers of felix.fontein.de] **********************************************************************
ok: [localhost]

TASK [Debug print __ff_ns] ********************************************************************************************
ok: [localhost] => {
    "msg": {
        "changed": false,
        "failed": false,
        "results": [
            {
                "name": "felix.fontein.de",
                "nameservers": [
                    "ns1.hostserv.eu.",
                    "ns2.hostserv.eu.",
                    "ns3.hostserv.eu."
                ]
            }
        ]
    }
}

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Can you please add a changelog fragment? Also would you mind making the same changes to the nameserver_record_info module?

hakong and others added 4 commits November 30, 2023 10:13
Removed required: and default:, added version_added.

Co-authored-by: Felix Fontein <[email protected]>
@hakong
Copy link
Contributor Author

hakong commented Nov 30, 2023

Thanks for your contribution! Can you please add a changelog fragment? Also would you mind making the same changes to the nameserver_record_info module?

I'll do my best. As you can probably see I'm not very experienced, so I'll take all the help I can get :)

@felixfontein
Copy link
Collaborator

Restarting CI (so that integration tests run).

@felixfontein felixfontein reopened this Dec 2, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks! One little update to the changelog fragment, then it should be done!

@felixfontein felixfontein merged commit a1dafd5 into ansible-collections:main Dec 12, 2023
@felixfontein
Copy link
Collaborator

@hakong thanks for your contribution!

@felixfontein
Copy link
Collaborator

I've decided to rename servers to server to adjust notation to the parameters of the lookups. The change is done in #178.

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