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

Update iptables.py #41245

Merged
merged 2 commits into from
Aug 17, 2018
Merged

Update iptables.py #41245

merged 2 commits into from
Aug 17, 2018

Conversation

dshuvar
Copy link
Contributor

@dshuvar dshuvar commented Jun 7, 2018

SUMMARY

Added to documentation the required variable 'protocol' for variable destination_port.

ISSUE TYPE

Docs Pull Request

COMPONENT NAME

iptables.py

ANSIBLE VERSION
ansible 2.5.4
ADDITIONAL INFORMATION
if does not use variable 'protocol' you can see error:
iptables v1.6.0: unknown option "--destination-port"
like in https://github.com/ansible/ansible-modules-extras/issues/2100
it means need notes about obligate variable in documentation of module.

added to documentation the required variable 'protocol' for variable destination_port.
@ansibot
Copy link
Contributor

ansibot commented Jun 7, 2018

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 7, 2018
Copy link
Contributor

@sebastiendarocha sebastiendarocha left a comment

Choose a reason for hiding this comment

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

You can't put a ":" on documation, it's yaml so it considers the line as a dict.
You have to encapsulate the line between quotes (see "description" of the parameter "to_ports")

@ansibot
Copy link
Contributor

ansibot commented Jun 7, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/iptables.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/system/iptables.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/system/iptables.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/system/iptables.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/system/iptables.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 101, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

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

lib/ansible/modules/system/iptables.py:0:0: E324 Value for "default" from the argument_spec ('append') for "action" does not match the documentation (None)
lib/ansible/modules/system/iptables.py:0:0: E324 Value for "default" from the argument_spec ('filter') for "table" does not match the documentation (None)
lib/ansible/modules/system/iptables.py:0:0: E324 Value for "default" from the argument_spec ('ignore') for "syn" does not match the documentation (None)
lib/ansible/modules/system/iptables.py:0:0: E324 Value for "default" from the argument_spec ('ipv4') for "ip_version" does not match the documentation (None)
lib/ansible/modules/system/iptables.py:0:0: E324 Value for "default" from the argument_spec ('present') for "state" does not match the documentation (None)
lib/ansible/modules/system/iptables.py:187:18: E302 DOCUMENTATION is not valid YAML

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

lib/ansible/modules/system/iptables.py:187:18: error DOCUMENTATION: syntax error: mapping values are not allowed here

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. labels Jun 7, 2018
@dshuvar
Copy link
Contributor Author

dshuvar commented Jun 7, 2018

@sebastiendarocha thank you, I forgot about double quotes and already fix it.

@sebastiendarocha
Copy link
Contributor

shipit

Thanx :)

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 7, 2018
@LinusU
Copy link
Contributor

LinusU commented Jun 7, 2018

shipit

👍

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 7, 2018
@resmo
Copy link
Contributor

resmo commented Jun 13, 2018

To me it looks like we should enforce this in the code (or fail early). Would this make sense?

@ansibot ansibot added small_patch needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed shipit This PR is ready to be merged by Core needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 21, 2018
@ansibot ansibot added shipit This PR is ready to be merged by Core needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed shipit This PR is ready to be merged by Core labels Jun 28, 2018
@ansibot ansibot added shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 6, 2018
@dshuvar
Copy link
Contributor Author

dshuvar commented Jul 9, 2018

It makes sense because if you regularly use this role you know how right use this option,
but if you saw or use in the first time this module you with a high probability make mistake.
Documentation and feature which you use must be been able to evident.

@resmo resmo merged commit 9d3b479 into ansible:devel Aug 17, 2018
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. shipit This PR is ready to be merged by Core small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants