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

Add full IPv6 support to win_dns_client - Fixes #55962 #57577

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@agowa338
Copy link
Contributor

commented Jun 8, 2019

SUMMARY

Add full IPv6 support to win_dns_client.

Fixes #55962

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_dns_client

ADDITIONAL INFORMATION

Fully implement ipv6 support within win_dns_client, former it was in a weird/undefined state and unintentionally kinda working.

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/windows/win_dns_client.py:22:161: E501 line too long (169 > 160 characters)

The test ansible-test sanity --test pslint [explain] failed with 10 errors:

lib/ansible/modules/windows/win_dns_client.ps1:116:30: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:121:48: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:122:61: MissingEndParenthesisInExpression Missing closing ')' in expression.
lib/ansible/modules/windows/win_dns_client.ps1:122:62: UnexpectedToken Unexpected token 'in' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:122:81: UnexpectedToken Unexpected token ')' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:123:5: UnexpectedToken Unexpected token '}' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:123:7: EmptyPipeElement An empty pipe element is not allowed.
lib/ansible/modules/windows/win_dns_client.ps1:162:1: UnexpectedToken Unexpected token '}' in expression or statement.
test/sanity/pslint/ignore.txt:44:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSAvoidUsingCmdletAliases" test
test/sanity/pslint/ignore.txt:47:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSUseApprovedVerbs" test

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/windows/win_dns_client.py:0:0: E305 DOCUMENTATION.options.dns_servers.alias: extra keys not allowed @ data['options']['dns_servers']['alias']. Got ['ipv4_addresses', 'ip_addresses', 'addresses']
lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be '2.9'. Currently StrictVersion ('0.0')

click here for bot help

Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.ps1 Outdated
Write-DebugLog "There are currently dns servers specified, but they should be absent."
return $false
} else {}
foreach($address in $current_dns) {

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 8, 2019

Contributor

Consider Compare-Object instead of all these loops

Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.ps1 Outdated

If($invalid_addresses.Count -gt 0) {
If([bool]$invalid_addresses) {
throw "Invalid IP address(es): ({0})" -f ($invalid_addresses -join ", ")

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 8, 2019

Contributor

Maybe this should be a warning since you check it in other places and never use invalid ips?

This comment has been minimized.

Copy link
@agowa338

agowa338 Jun 8, 2019

Author Contributor

Are you sure, shouldn't the play fail if an invalid address is specified?
The checks on other places are just to avoid weird behaviours, even though they should never be hit.
This one is part of ansible play error handling.

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 8, 2019

Contributor

If there is a list of IPs and some are bad I would want to get a warning and set the good ones as a user, but that's just me

This comment has been minimized.

Copy link
@agowa338

agowa338 Jun 8, 2019

Author Contributor

One could (and I think also should) use the ip address jinja2 filters to validate and if you hardcoded values at least I want it to stick to a fail fast and fail forward approach...

Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.py
Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.py
Show resolved Hide resolved lib/ansible/modules/windows/win_dns_client.py Outdated

@agowa338 agowa338 force-pushed the agowa338:patch-9 branch 3 times, most recently from be618d8 to 554f411 Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/windows/win_dns_client.py:22:161: E501 line too long (169 > 160 characters)

The test ansible-test sanity --test pslint [explain] failed with 11 errors:

lib/ansible/modules/windows/win_dns_client.ps1:115:30: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:120:48: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:121:61: MissingEndParenthesisInExpression Missing closing ')' in expression.
lib/ansible/modules/windows/win_dns_client.ps1:121:62: UnexpectedToken Unexpected token 'in' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:121:81: UnexpectedToken Unexpected token ')' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:122:5: UnexpectedToken Unexpected token '}' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:122:7: EmptyPipeElement An empty pipe element is not allowed.
lib/ansible/modules/windows/win_dns_client.ps1:160:1: UnexpectedToken Unexpected token '}' in expression or statement.
test/sanity/pslint/ignore.txt:44:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSAvoidUsingCmdletAliases" test
test/sanity/pslint/ignore.txt:47:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSUseApprovedVerbs" test
test/sanity/pslint/ignore.txt:48:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSUseDeclaredVarsMoreThanAssignments" test

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/windows/win_dns_client.py:0:0: E305 DOCUMENTATION.options.dns_servers.alias: extra keys not allowed @ data['options']['dns_servers']['alias']. Got ['ipv4_addresses', 'ip_addresses', 'addresses']
lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be '2.9'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

@agowa338 agowa338 force-pushed the agowa338:patch-9 branch from 554f411 to 6d4e239 Jun 8, 2019

@ansibot ansibot removed the ci_verified label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test pslint [explain] failed with 11 errors:

lib/ansible/modules/windows/win_dns_client.ps1:115:30: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:120:48: MissingEndCurlyBrace Missing closing '}' in statement block or type definition.
lib/ansible/modules/windows/win_dns_client.ps1:121:77: MissingEndParenthesisInExpression Missing closing ')' in expression.
lib/ansible/modules/windows/win_dns_client.ps1:121:78: UnexpectedToken Unexpected token 'in' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:121:97: UnexpectedToken Unexpected token ')' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:122:5: UnexpectedToken Unexpected token '}' in expression or statement.
lib/ansible/modules/windows/win_dns_client.ps1:122:7: EmptyPipeElement An empty pipe element is not allowed.
lib/ansible/modules/windows/win_dns_client.ps1:160:1: UnexpectedToken Unexpected token '}' in expression or statement.
test/sanity/pslint/ignore.txt:44:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSAvoidUsingCmdletAliases" test
test/sanity/pslint/ignore.txt:47:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSUseApprovedVerbs" test
test/sanity/pslint/ignore.txt:48:1: A102 Remove since "lib/ansible/modules/windows/win_dns_client.ps1" passes "PSUseDeclaredVarsMoreThanAssignments" test

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/windows/win_dns_client.py:0:0: E305 DOCUMENTATION.options.dns_servers.alias: extra keys not allowed @ data['options']['dns_servers']['alias']. Got ['ipv4_addresses', 'ip_addresses', 'addresses']
lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be '2.9'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

@agowa338 agowa338 force-pushed the agowa338:patch-9 branch from 6d4e239 to 1b22072 Jun 8, 2019

@ansibot ansibot removed the ci_verified label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

@ansibot ansibot added the test label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/windows/win_dns_client.py:0:0: E305 DOCUMENTATION.options.dns_servers.alias: extra keys not allowed @ data['options']['dns_servers']['alias']. Got ['ipv4_addresses', 'ip_addresses', 'addresses']
lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be '2.9'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

@agowa338 agowa338 force-pushed the agowa338:patch-9 branch from dad4b23 to 0f9e585 Jun 8, 2019

@ansibot ansibot removed the ci_verified label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be None. Currently '2.9'

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

@ansibot ansibot removed the ci_verified label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/windows/win_dns_client.py:0:0: E309 version_added for new option (dns_servers) should be None. Currently '2.9'

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

Remove version_added again, to hopefully make ansibot happy. Even tho…
…ugh it was added as a response to the bot...
@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

try replacing the quotes in the version_added: "2.9" to single ones version_added: '2.9'
in all other modules, they have a single

@agowa338

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

That is not the problem. Ansibot says it is recognized. Maybe I managed to confuse the ci check, or it thinks it is not new, as the old name is still there as an alias.

It specifically says:

version_added for new option (dns_servers) should be None. Currently '2.9'

So it wants it to be None or absent and not '2.9'...

I just tried it locally, the check ansible-test sanity --requirements --test validate-modules --base-branch upstream/devel win_dns_client does also not like it with single quotes. It only succeeds if it is absent.

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@agowa338 @ShachafGoldstein having "2.9" or '2.9' is fine, the reason why it is saying it should be None is that you have just aliased an existing option. While the new name has been added in 2.9, it can be called using one of the aliases for the older versions. What you have documented is correct. I'm hoping to have a look at this at some point soon.

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.