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

added datetime type to argspec fix#31017 #31130

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@Jeyanthinath

Jeyanthinath commented Sep 30, 2017

Adding datetime option to argspec

ISSUE TYPE
  • Feature Pull Request
    issue - #31017
COMPONENT NAME

module_utils/basic.py

ANSIBLE VERSION
ansible 2.5.0 (argspec_datetime ca99ff80dd) last updated 2017/09/30 20:42:18 (GMT +550)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/jeyanthinath/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jeyanthinath/Documents/git/ansible/lib/ansible
  executable location = /home/jeyanthinath/Documents/git/ansible/bin/ansible
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]
@sivel

This comment has been minimized.

Member

sivel commented Sep 30, 2017

dateutil is not in the standard library, and it’s import here is not protected by a try/except.

Additionally since this is going to be core functionality, it can’t rely on a non standard library module.

I’d recommend requiring a specific date format. The best bet is one that matches YAML date format parsing.

http://yaml.org/type/timestamp.html

@Jeyanthinath Jeyanthinath force-pushed the Jeyanthinath:argspec_datetime branch Sep 30, 2017

@Jeyanthinath

This comment has been minimized.

Jeyanthinath commented Sep 30, 2017

@sivel but in this approach , I feel only now datetime.datetime format can be supported instead of simple string. This will give extra efforts to the end-user !

@sivel

This comment has been minimized.

Member

sivel commented Sep 30, 2017

I understand your concern, unfortunately as I mention, a solution requiring no 3rd party modules will be required. Likely you’ll need to implement the regex from that doc I mention and create a date time from the match using datetime.datetime

@bcoca bcoca removed the needs_triage label Oct 2, 2017

@bcoca

This comment has been minimized.

Member

bcoca commented Oct 2, 2017

@Jeyanthinath I agree with @sivel here, we don't want to force all targets that have modules using this feature to also require extra libraries.

@sivel

This comment has been minimized.

Member

sivel commented Oct 2, 2017

This is the exact regex from pyyaml: https://github.com/yaml/pyyaml/blob/master/lib/yaml/constructor.py#L292-L332

We can't rely on pyyaml remotely either, as pyyaml isn't a minimum dependency for targets.

Adding datetime option to argspec fixes #31017
Signed-off-by: Jeyanthinath <jeyanthinath10@gmail.com>

@Jeyanthinath Jeyanthinath force-pushed the Jeyanthinath:argspec_datetime branch to 5c72ecb Oct 3, 2017

@ansibot ansibot added needs_ci and removed needs_revision labels Oct 3, 2017

@Jeyanthinath

This comment has been minimized.

Jeyanthinath commented Oct 3, 2017

@sivel can you review my changes

@ansibot ansibot removed the needs_ci label Oct 3, 2017

@sivel

This comment has been minimized.

Member

sivel commented Oct 3, 2017

I do have a few concerns/questions. The code in pyyaml is licensed as MIT. The code in basic.py is licensed BSD.

I don't know whether it is permitted to simply copy and paste the code from pyyaml into ansible.module_utils.basic

Also, and this is a question for @bcoca what are our plans for importable type functions? I know we can already specify a callable as the type. Is it feasible that we continue adding these as string types that get mapped to a callable, or should we think about making new filters callables by default, and having the modules import them?

Also, we may need to have some questions surrounding the legality above.

@bcoca

This comment has been minimized.

Member

bcoca commented Oct 3, 2017

IANAL, so we'll have to find one to answer the license legality issue.

as for 'callable' i was under the impression that we were removing that type.

@sivel

This comment has been minimized.

Member

sivel commented Oct 3, 2017

as for 'callable' i was under the impression that we were removing that type

Interesting, because I know we were looking to add types that we didn't consider part of the core features, that may or may not require 3rd party libraries, specifically around network modules. And that was the recommended approach we gave the other user. I am fine either way, I was just making sure we didn't grow that class too much. (#17731)

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 3, 2017

CI failure in integration tests. Most appear to be due to syntax errors reported by the sanity tests:

lib/ansible/module_utils/basic.py:192:376: SyntaxError: ur'''^(?P[0-9][0-9][0-9][0-9])

@mattclay mattclay added the ci_verified label Oct 3, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 3, 2017

The test ansible-test compile --python 3.5 [?] failed with the following error:

lib/ansible/module_utils/basic.py:192:376: SyntaxError: ur'''^(?P<year>[0-9][0-9][0-9][0-9])

The test ansible-test compile --python 3.6 [?] failed with the following error:

lib/ansible/module_utils/basic.py:192:376: SyntaxError: ur'''^(?P<year>[0-9][0-9][0-9][0-9])

The test ansible-test sanity --test ansible-var-precedence-check [?] failed with the following error:

Command "test/sanity/code-smell/ansible-var-precedence-check.py" returned exit status 1.
>>> Standard Output
CHECKING: extra_vars (var passed via the cli)
ERROR !!!
playbook failed (rc=250), stdout: 'b'the full traceback was:\n\nTraceback (most recent call last):\n  File "/root/src/github.com/ansible/ansible/bin/ansible-playbook", line 88, in <module>\n    mycli = getattr(__import__("ansible.cli.%s" % sub, fromlist=[myclass]), myclass)\n  File "/root/src/github.com/ansible/ansible/lib/ansible/cli/__init__.py", line 41, in <module>\n    from ansible.parsing.dataloader import DataLoader\n  File "/root/src/github.com/ansible/ansible/lib/ansible/parsing/dataloader.py", line 20, in <module>\n    from ansible.module_utils.basic import is_executable\n  File "/root/src/github.com/ansible/ansible/lib/ansible/module_utils/basic.py", line 192\n    ur\'\'\'^(?P<year>[0-9][0-9][0-9][0-9])\n        -(?P<month>[0-9][0-9]?)\n        -(?P<day>[0-9][0-9]?)\n        (?:(?:[Tt]|[ \\t]+)\n        (?P<hour>[0-9][0-9]?)\n        :(?P<minute>[0-9][0-9])\n        :(?P<second>[0-9][0-9])\n        (?:\\.(?P<fraction>[0-9]*))?\n        (?:[ \\t]*(?P<tz>Z|(?P<tz_sign>[-+])(?P<tz_hour>[0-9][0-9]?)\n        (?::(?P<tz_minute>[0-9][0-9]))?))?)?$\'\'\', re.X)\n                                        \n                               \n                             \n                          \n                             \n                               \n                               \n                                   \n                                                                   \n                                               ^\nSyntaxError: invalid syntax\n'' stderr: 'b'ERROR! Unexpected Exception, this is probably a bug: invalid syntax (basic.py, line 192)\n''
feature: extra_vars failed

The test ansible-test sanity --test pylint [?] failed with the following errors:

lib/ansible/module_utils/basic.py:192:0: syntax-error invalid syntax
lib/ansible/modules/cloud/amazon/ec2_ami_find.py:352:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_eip.py:366:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_key.py:136:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_key.py:166:32: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:168:31: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:172:28: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:211:36: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:213:35: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:217:32: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:238:28: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:240:27: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_key.py:244:24: undefined-variable Undefined variable 'time'
lib/ansible/modules/cloud/amazon/ec2_metric_alarm.py:300:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_scaling_policy.py:177:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_snapshot.py:270:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_vol.py:528:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ec2_vpc_net.py:238:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/ecs_ecr.py:325:30: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/ecs_ecr.py:348:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/efs.py:585:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/iam.py:246:91: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/iam.py:256:104: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/iam.py:270:95: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/iam.py:279:113: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/iam.py:289:101: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/amazon/iam.py:624:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/kinesis_stream.py:1120:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/rds.py:1356:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/amazon/redshift.py:475:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_instance.py:1018:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_instance_facts.py:290:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_portforward.py:412:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_securitygroup.py:199:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_securitygroup_rule.py:395:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/cloudstack/cs_snapshot_policy.py:359:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_auth.py:64:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_auth.py:77:47: undefined-variable Undefined variable 'traceback'
lib/ansible/modules/cloud/openstack/os_flavor_facts.py:208:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_floating_ip.py:175:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_floating_ip.py:224:49: undefined-variable Undefined variable 'remove_values'
lib/ansible/modules/cloud/openstack/os_group.py:136:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_image.py:161:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_image_facts.py:151:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_ironic.py:234:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_ironic_inspect.py:127:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_ironic_node.py:252:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_keypair.py:127:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_keystone_domain.py:146:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_keystone_role.py:102:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_keystone_service.py:156:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_network.py:185:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_networks_facts.py:141:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_nova_flavor.py:220:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_nova_host_aggregate.py:125:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_object.py:124:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_port.py:333:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_port_facts.py:213:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_project.py:160:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_project_facts.py:133:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_recordset.py:173:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_router.py:319:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_security_group.py:107:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_security_group_rule.py:301:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_server.py:732:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_server_facts.py:83:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_server_group.py:138:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_server_volume.py:105:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_stack.py:230:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_subnet.py:275:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_subnets_facts.py:154:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_user_facts.py:141:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_user_group.py:84:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_user_role.py:141:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_volume.py:176:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/cloud/openstack/os_zone.py:174:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/apt_repository.py:304:30: undefined-variable Undefined variable 'get_exception'
lib/ansible/modules/packaging/os/apt_repository.py:305:96: undefined-variable Undefined variable 'unicode'
lib/ansible/modules/packaging/os/apt_repository.py:404:15: undefined-variable Undefined variable 'json'
lib/ansible/modules/packaging/os/apt_repository.py:480:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/apt_repository.py:523:14: undefined-variable Undefined variable 'get_exception'
lib/ansible/modules/packaging/os/apt_repository.py:524:63: undefined-variable Undefined variable 'unicode'
lib/ansible/modules/packaging/os/apt_repository.py:546:18: undefined-variable Undefined variable 'get_exception'
lib/ansible/modules/packaging/os/apt_repository.py:547:33: undefined-variable Undefined variable 'unicode'
lib/ansible/modules/packaging/os/apt_rpm.py:153:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/dpkg_selections.py:45:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/homebrew_tap.py:206:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/layman.py:229:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/macports.py:198:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/opkg.py:170:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/pkg5.py:67:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/pkg5.py:98:12: undefined-variable Undefined variable 're'
lib/ansible/modules/packaging/os/pkg5.py:99:29: undefined-variable Undefined variable 're'
lib/ansible/modules/packaging/os/pkg5_publisher.py:80:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/pkgin.py:331:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/pkgutil.py:127:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/portage.py:451:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/slackpkg.py:171:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/svr4pkg.py:177:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/swdepot.py:133:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/urpmi.py:191:13: undefined-variable Undefined variable 'AnsibleModule'
lib/ansible/modules/packaging/os/zypper_repository.py:305:13: undefined-variable Undefined variable 'AnsibleModule'
test/units/modules/network/cumulus/test_nclu.py:98:7: undefined-variable Undefined variable 'os'

click here for bot help

@Jeyanthinath

This comment has been minimized.

Jeyanthinath commented Oct 4, 2017

@sivel I also looked have the same license concern ! Shall I rewrite the entire code of my own instead of using that pyyaml code ?

@mattclay I will check on this and update , but ironically code is running for me 🤔 , May be I am missing something over there

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 4, 2017

@Jeyanthinath The syntax errors are on Python 3.x only, so make sure you're testing that.

@Jeyanthinath

This comment has been minimized.

Jeyanthinath commented Oct 10, 2017

@mattclay as Licensing concern pointed out by @sivel , shall I re-write the code from scratch to support those ? or is it ok to go with the code from pyyaml library ?

@sivel

This comment has been minimized.

Member

sivel commented Oct 10, 2017

It's likely going to take a while to get legal verification about inclusion.

I'd advise great care if you do plan to rewrite.

@sivel

This comment has been minimized.

Member

sivel commented Oct 11, 2017

I've contacted the FSF, and confirmed that the BSD license (the license of basic.py) and the MIT license are compatible, and that you can copy part or whole of a MIT licensed file into a BSD licensed file, but must retain the copyright notice of the MIT file. You will need to reference in the docstring the copyright of that function, as well as site where it came from.

Someone from Red Hat may need to validate this as well. I suppose effectively it's on them ultimately.

@ansibot ansibot added the stale_ci label Oct 11, 2017

@abadger

This comment has been minimized.

Member

abadger commented Oct 16, 2017

@sivel, yep, the ideas in #17731 are still the plan. Talked to bcoca on slack to clarify that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment