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

Adjust parameters to GoDaddy get_zone method #1967

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

Circuitsoft
Copy link

Other drivers take the ID, not the domain name. This puts the GoDaddy driver in-line with how other drivers work.

Changes Title (replace this with a logical title for your changes)

Description

Saltstack has been throwing errors when working with GoDaddy dns entries because the driver takes different parameters than other drivers it was designed around. This fixes the interface to put it more in-line with other drivers.

Status

done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

Other drivers take the ID, not the domain name. This puts the GoDaddy
driver in-line with how other drivers work.
@Kami
Copy link
Member

Kami commented Oct 28, 2023

@Circuitsoft Thanks for the contribution.

Since this is a breaking change, can you please add an entry to docs/upgrade_notes.rst file? You should be able to find some examples on how to word an entry in that file.

Thanks.

@Kami
Copy link
Member

Kami commented Dec 3, 2023

@Circuitsoft It appears tests are also failing.

 >        domain, = [x.domain for x in self.list_zones() if x.id == zone_id]
 E       ValueError: not enough values to unpack (expected 1, got 0)

Can you please also look into this when you get a chance? Thanks

:type zone_id: ``str``

:rtype: :class:`Zone`
"""
result = self.connection.request("/v1/domains/%s/" % zone_id).object
domain, = [x.domain for x in self.list_zones() if x.id == zone_id]
Copy link
Member

Choose a reason for hiding this comment

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

This will throw in case no matching items are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants