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

Improve Docs In azure_rm_virtualnetwork #1203

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

timway
Copy link
Contributor

@timway timway commented Jun 30, 2023

SUMMARY
  • The DNS Servers should be a list of strings
  • Remove the trailing comma in the dict() call for dns_servers
  • The boolean values should default to a boolean value in the docs
  • Clean up excess quotation in some of the examples
ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME
azure_rm_virtualnetwork
ADDITIONAL INFORMATION
ansible-doc --type module azure.azcollection.azure_rm_virtualnetwork

@@ -218,7 +222,7 @@ def __init__(self):
state=dict(type='str', default='present', choices=['present', 'absent']),
location=dict(type='str'),
address_prefixes_cidr=dict(type='list', aliases=['address_prefixes']),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
address_prefixes_cidr=dict(type='list', aliases=['address_prefixes']),
address_prefixes_cidr=dict(type='list', elements='str', aliases=['address_prefixes']),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fred-sun I've updated the code to reflect this change. Thank you for the comment!

@@ -218,7 +222,7 @@ def __init__(self):
state=dict(type='str', default='present', choices=['present', 'absent']),
location=dict(type='str'),
address_prefixes_cidr=dict(type='list', aliases=['address_prefixes']),
dns_servers=dict(type='list',),
dns_servers=dict(type='list'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dns_servers=dict(type='list'),
dns_servers=dict(type='list', elements='str'),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fred-sun I've updated the code to reflect this change. Thank you for the comment!

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors documentation-pr Improvements or additions to documentation labels Jul 3, 2023
* The DNS Servers should be a list of strings
* Remove the trailing comma in the `dict()` call for `dns_servers`
* The boolean values should default to a boolean value in the docs
* Clean up excess quotation in some of the examples
* Incorporate PR feedback from @Fred-sun correctly indicated that I didn't apply the elements parameter to the argument spec for `address_prefixes_cidr` and `dns_servers`
* This helps Ansible validate the list elements are the correct type for the 2 affected module parameters
@timway timway force-pushed the vnet-dns-server-doc-tweaks branch from 7e1ef70 to d2d4edf Compare July 5, 2023 22:36
@Fred-sun
Copy link
Collaborator

Fred-sun commented Jul 6, 2023

@timway Please delete the corresponding line in the following file

Line 228 -- test/sanity/ignore-2.10.txt  plugins/modules/azure_rm_virtualnetwork.py validate-modules:parameter-list-no-elements
Line 228 -- test/sanity/ignore-2.11.txt  plugins/modules/azure_rm_virtualnetwork.py validate-modules:parameter-list-no-elements
Line 228 -- test/sanity/ignore-2.12.txt  plugins/modules/azure_rm_virtualnetwork.py validate-modules:parameter-list-no-elements
Line 227 -- test/sanity/ignore-2.13.txt  plugins/modules/azure_rm_virtualnetwork.py validate-modules:parameter-list-no-elements
Line 224 -- test/sanity/ignore-2.14.txt  plugins/modules/azure_rm_virtualnetwork.py validate-modules:parameter-list-no-elements

@timway timway force-pushed the vnet-dns-server-doc-tweaks branch from b6432e2 to d2d4edf Compare July 6, 2023 13:03
@Fred-sun
Copy link
Collaborator

Fred-sun commented Jul 7, 2023

@timway Please update the corresponding Ignore*.txt, because we added elements in document and args, which should be deleted (ignore line)accordingly! Thank you very much!

* As @Fred-sun pointed out a number of the sanity tests no longer need to ignore a failure condition with this PR
@Fred-sun
Copy link
Collaborator

kindly ping!

@timway
Copy link
Contributor Author

timway commented Jul 13, 2023

@Fred-sun I believe I addressed the ignores in the most recent commit.

@Fred-sun
Copy link
Collaborator

@timway Sorry, it has been added, I will push forward the merge.

@Fred-sun
Copy link
Collaborator

@xuzhang3 ready_for_review

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged and removed work in In trying to solve, or in working with contributors labels Jul 13, 2023
@xuzhang3 xuzhang3 merged commit 76c1cf2 into ansible-collections:dev Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-pr Improvements or additions to documentation medium_priority Medium priority ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants