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

DNS Made Easy: Fixed monitor (DNS failover) record property handling #56880

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@EagleIJoe
Copy link
Contributor

commented May 23, 2019

SUMMARY

The handling of failover record property has been corrected. These changes ensure that the monitor property is only applied for the applicable DNS records (A and CNAME in this case). In the process, the handling has been made idempotent.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

dnsmadeeasy

ADDITIONAL INFORMATION

While doing other development work (TXT record handling), the monitor would also try to apply to such records. This lead to the initial investigation and root cause (wrong indention). Once that was fixed, the modification of the records itself was not working correctly and throwing 404s from the API.
Hence the following:

  • Monitor property creation has been separated out as it is needed whenever monitor parameter is specified.
  • CNAME record type has been added as a further failover candidate
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@EagleIJoe this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@EagleIJoe EagleIJoe force-pushed the EagleIJoe:pr_dme_fix_monitor_handling branch 2 times, most recently from c94be0f to 9b67eab May 23, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

The test ansible-test sanity --test pylint [explain] failed with 6 errors:

lib/ansible/modules/net_tools/dnsmadeeasy.py:364:1: trailing-whitespace Trailing whitespace
lib/ansible/modules/net_tools/dnsmadeeasy.py:629:7: singleton-comparison Comparison to None should be 'expr is not None'
lib/ansible/modules/net_tools/dnsmadeeasy.py:647:49: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/net_tools/dnsmadeeasy.py:660:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/net_tools/dnsmadeeasy.py:661:11: singleton-comparison Comparison to True should be just 'expr'
lib/ansible/modules/net_tools/dnsmadeeasy.py:668:0: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

lib/ansible/modules/net_tools/dnsmadeeasy.py:364:2: W291 trailing whitespace
lib/ansible/modules/net_tools/dnsmadeeasy.py:629:33: E711 comparison to None should be 'if cond is not None:'
lib/ansible/modules/net_tools/dnsmadeeasy.py:632:17: E128 continuation line under-indented for visual indent
lib/ansible/modules/net_tools/dnsmadeeasy.py:633:17: E128 continuation line under-indented for visual indent
lib/ansible/modules/net_tools/dnsmadeeasy.py:659:11: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/dnsmadeeasy.py:660:1: W293 blank line contains whitespace
lib/ansible/modules/net_tools/dnsmadeeasy.py:661:38: E712 comparison to True should be 'if cond is True:' or 'if cond:'
lib/ansible/modules/net_tools/dnsmadeeasy.py:662:11: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/dnsmadeeasy.py:664:11: E114 indentation is not a multiple of four (comment)
lib/ansible/modules/net_tools/dnsmadeeasy.py:665:11: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/dnsmadeeasy.py:666:15: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/dnsmadeeasy.py:667:19: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/dnsmadeeasy.py:668:1: W293 blank line contains whitespace

click here for bot help

@ansibot ansibot added the ci_verified label May 23, 2019

@EagleIJoe EagleIJoe force-pushed the EagleIJoe:pr_dme_fix_monitor_handling branch from 9b67eab to 074868b May 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@EagleIJoe this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@EagleIJoe EagleIJoe changed the title [WIP] DNS Made Easy: Fixed monitor (DNS failover) record property handling DNS Made Easy: Fixed monitor (DNS failover) record property handling May 26, 2019

@EagleIJoe EagleIJoe closed this May 26, 2019

@EagleIJoe EagleIJoe reopened this May 26, 2019

@EagleIJoe EagleIJoe force-pushed the EagleIJoe:pr_dme_fix_monitor_handling branch from 074868b to 08c3cf8 May 26, 2019

@EagleIJoe EagleIJoe marked this pull request as ready for review May 26, 2019

@felixfontein
Copy link
Contributor

left a comment

I haven't checked everything; but one comment: if you break backwards compatibility, then this can't be backported to stable-2.8 or earlier. So maybe it's better to split this up into two PRs, one for changing systemDescription (which will appear in 2.9), and one for the bugfix itself (which can be backported).

@ansibot ansibot removed the needs_triage label May 26, 2019

@EagleIJoe EagleIJoe changed the title DNS Made Easy: Fixed monitor (DNS failover) record property handling [WIP] DNS Made Easy: Fixed monitor (DNS failover) record property handling May 27, 2019

@ansibot ansibot added the WIP label May 27, 2019

EagleIJoe and others added some commits May 23, 2019

DNS Failover correction
Monitors are now handled idempotently
Monitors only get touched where applicable

Removed default value for systemDescription to ensure requirement
required_if fails to check otherwise

added required changelog fragement
Update changelogs/fragments/56880-dnsmadeeasy-fix_monitor_handling.yml
Co-Authored-By: Felix Fontein <felix@fontein.de>
Update changelogs/fragments/56880-dnsmadeeasy-fix_monitor_handling.yml
Co-Authored-By: Felix Fontein <felix@fontein.de>
check for record_type param for monitor creation
return new_monitor if no current record exists

@EagleIJoe EagleIJoe force-pushed the EagleIJoe:pr_dme_fix_monitor_handling branch from 489aa12 to 6ca3cc0 May 28, 2019

@EagleIJoe EagleIJoe changed the title [WIP] DNS Made Easy: Fixed monitor (DNS failover) record property handling DNS Made Easy: Fixed monitor (DNS failover) record property handling May 28, 2019

@ansibot ansibot removed the WIP label May 28, 2019

@ansibot ansibot added the stale_ci label Jun 5, 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.