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

validate-modules: new module: fail if 'type' isn't documented #56534

Merged

Conversation

Projects
None yet
5 participants
@pilou-
Copy link
Contributor

commented May 16, 2019

SUMMARY

validate-modules: new module: fail if type isn't documented and type in arg_spec isn't str.

  • type isn't required in documentation when choices is set (different types can be used with choices, see)
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

validate-modules

ADDITIONAL INFORMATION

Related: #56312

Locally tested on top of #51746 (70435d6):

$ python test/sanity/validate-modules/validate-modules  --arg-spec lib/ansible/modules/cloud/scaleway/scaleway_lb_facts.py --base-branch upstream/devel
lib/ansible/modules/cloud/scaleway/scaleway_lb_facts.py:0:0: E337 Argument 'api_version' in argument_spec implies type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/scaleway/scaleway_lb_facts.py:0:0: E337 Argument 'region' in argument_spec implies type as 'str' but documentation doesn't define type
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@pilou- pilou- changed the title [WIP] validate-modules: new module: fail if 'type' isn't documented validate-modules: new module: fail if 'type' isn't documented May 16, 2019

@ansibot ansibot added needs_revision and removed WIP labels May 16, 2019

@pilou-

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Sanity errors below seems unrelated:

18:06 ERROR: Found 2 validate-modules issue(s) which need to be resolved:
18:06 ERROR: lib/ansible/modules/cloud/vmware/vsphere_copy.py:0:0: E309 version_added for new option (host) should be '2.9'. Currently None (0%)
18:06 ERROR: lib/ansible/modules/cloud/vmware/vsphere_copy.py:0:0: E309 version_added for new option (login) should be '2.9'. Currently None (0%)
@mattclay

This comment has been minimized.

Copy link
Member

commented May 18, 2019

@pilou- Those errors have been resolved. I've restarted the failed tests.

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch from 549ac40 to 0c29c05 May 18, 2019

@ansibot ansibot added core_review and removed needs_revision labels May 18, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

(I think you have a typo, you mean type='str' and not type='list' for choices.)

About the implications: suboptions can also be used with type='dict', and choices can also be used with other types (such as int).

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch from 0c29c05 to 10caa28 May 20, 2019

@pilou-

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

About the implications: suboptions can also be used with type='dict', and choices can also be used with other types (such as int).

PR updated: type is now required when suboptions is set

@ansibot ansibot added needs_revision and removed core_review labels May 21, 2019

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch from 10caa28 to 06eebf1 May 21, 2019

@samdoran samdoran removed the needs_triage label May 21, 2019

@mattclay

This comment has been minimized.

Copy link
Member

commented May 22, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Components

test/sanity/validate-modules/main.py
support: core
maintainers:

Metadata

waiting_on: pilou-
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: unstable
shippable_status: failure
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch from 06eebf1 to 82697d4 May 22, 2019

@mattclay mattclay requested a review from sivel Jun 7, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Before merging, CI should be run again. Due to recently merged module renames, it could be that some entries in ignore.txt need to be added / removed.

@mattclay

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

CI restarted. However, there's also a conflict in the ignore file to resolve.

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch 2 times, most recently from 72c7ae7 to 41c3568 Jun 10, 2019

@ansibot ansibot removed the needs_rebase label Jun 10, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

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

lib/ansible/modules/cloud/amazon/ec2_ami_info.py:0:0: E337 Argument 'executable_users' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_ami_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_ami_info.py:0:0: E337 Argument 'image_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_ami_info.py:0:0: E337 Argument 'owners' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_asg_info.py:0:0: E337 Argument 'name' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_asg_info.py:0:0: E337 Argument 'tags' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_customer_gateway_info.py:0:0: E337 Argument 'customer_gateway_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_customer_gateway_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_eip_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_eni_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_group_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_instance_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_instance_info.py:0:0: E337 Argument 'instance_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_lc_info.py:0:0: E337 Argument 'name' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_lc_info.py:0:0: E338 Argument 'sort' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_lc_info.py:0:0: E338 Argument 'sort_end' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_lc_info.py:0:0: E338 Argument 'sort_order' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_lc_info.py:0:0: E338 Argument 'sort_start' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_placement_group_info.py:0:0: E337 Argument 'names' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_snapshot_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_snapshot_info.py:0:0: E337 Argument 'owner_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_snapshot_info.py:0:0: E337 Argument 'restorable_by_user_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_snapshot_info.py:0:0: E337 Argument 'snapshot_ids' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/ec2_vol_info.py:0:0: E337 Argument 'filters' in argument_spec defines type as 'dict' but documentation doesn't define type

click here for bot help

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch 2 times, most recently from 4bdee9f to 0e28c26 Jun 10, 2019

@pilou-

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@mattclay sivel won't be available the next week or two, could someone else review ?

@ansibot ansibot added core_review and removed needs_revision labels Jun 10, 2019

@mattclay

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@pilou- I restarted the tests to see what new ignores are needed. Add those and I'll go ahead and merge, so the PR doesn't need to be updated again.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

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

lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'artifacts' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'cache' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'environment' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'source' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'tags' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'timeout_in_minutes' in argument_spec defines type as 'int' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E337 Argument 'vpc_config' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E338 Argument 'description' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E338 Argument 'encryption_key' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E338 Argument 'name' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E338 Argument 'service_role' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codebuild.py:0:0: E338 Argument 'state' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E337 Argument 'artifact_store' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E337 Argument 'name' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E337 Argument 'role_arn' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E337 Argument 'stages' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E337 Argument 'version' in argument_spec defines type as 'int' but documentation doesn't define type
lib/ansible/modules/cloud/amazon/aws_codepipeline.py:0:0: E338 Argument 'state' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'app_engine_http_target' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'description' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'http_target' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'name' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'pubsub_target' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'region' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'retry_config' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'schedule' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'state' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job.py:0:0: E337 Argument 'time_zone' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_cloudscheduler_job_facts.py:0:0: E337 Argument 'region' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'accelerator_type' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'cidr_block' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'description' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'labels' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'name' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'network' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'scheduling_config' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'state' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'tensorflow_version' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node.py:0:0: E337 Argument 'zone' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/cloud/google/gcp_tpu_node_facts.py:0:0: E337 Argument 'zone' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/network/checkpoint/cp_network.py:0:0: E337 Argument 'color' in argument_spec defines type as 'str' but documentation doesn't define type

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 14, 2019

pilou- added some commits May 16, 2019

validate-modules: new module: fail if 'type' isn't documented
Don't require 'type' when:
- parameter name starts with an underscore

@pilou- pilou- force-pushed the pilou-:metadata_check_parameter_type_new_module branch from 0e28c26 to f7ee85b Jun 14, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jun 14, 2019

@pilou-

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@mattclay PR updated.

@mattclay mattclay merged commit 3fb4c91 into ansible:devel Jun 14, 2019

1 check passed

Shippable Run 127665 status is SUCCESS.
Details
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

While trying to remove the docker modules from the ignore list, I found a potential bug in the validation. For docker_network, it yields one error:

ERROR: lib/ansible/modules/cloud/docker/docker_network.py:0:0: E337 Argument 'network_name' in argument_spec defines type as 'str' but documentation doesn't define type

But network_name is an alias for name, which has its type documented. The problem is apparently that the documentation says the option name is name and its alias is network_name, while argument_spec says the option is network_name with alias name. See https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/docker/docker_network.py#L23-L29 and https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/docker/docker_network.py#L634

The main question for me is now: is it allowed to switch option/alias order between argument_spec and docs? If yes, there's a bug in this sanity test. If no, another sanity check would be needed.

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.