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

move some of basic into common #48078

Merged
merged 9 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@acalm
Contributor

acalm commented Nov 5, 2018

SUMMARY

Moving some tiny/trivial bits out of basic into common/file.py and common/sys_info.py. Following the categorization by @abadger https://gist.github.com/abadger/3edc108f5a5b88c1a9a46c4869d778fd and some irc disussions. Keeping PRs small and going for the easy stuff first.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/basic.py

ANSIBLE VERSION
ansible 2.8.0.dev0 (basic-to-common 7af337d1ba) last updated 2018/11/05 10:26:13 (GMT +200)
  config file = None
  configured module search path = ['/home/acalm/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/acalm/venv/ansible/lib/python3.6/site-packages/ansible
  executable location = /home/acalm/venv/ansible/bin/ansible
  python version = 3.6.5 (default, Apr 20 2018, 08:08:06) [GCC 6.4.0]

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 5, 2018

Hi @acalm, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 5, 2018

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

lib/ansible/module_utils/common/sys_info.py:85:18: undefined-variable Undefined variable 'get_all_subclasses'
lib/ansible/module_utils/common/sys_info.py:89:18: undefined-variable Undefined variable 'get_all_subclasses'

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

lib/ansible/modules/system/hostname.py:43:0: ImportError: cannot import name get_distribution_version

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

lib/ansible/modules/system/hostname.py:43:0: ImportError: cannot import name get_distribution_version

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

lib/ansible/modules/system/hostname.py:43:0: ImportError: cannot import name 'get_distribution_version'

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

lib/ansible/modules/system/hostname.py:43:0: ImportError: cannot import name 'get_distribution_version'

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

lib/ansible/modules/system/hostname.py:43:0: ImportError: cannot import name 'get_distribution_version' from 'ansible.module_utils.basic' (/root/ansible/test/runner/.tox/import/lib/ansible/module_utils/basic.py)

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

lib/ansible/module_utils/common/sys_info.py:4:1: E302 expected 2 blank lines, found 1

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

lib/ansible/modules/system/hostname.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'cannot import name 'get_distribution_version''

click here for bot help

@ansibot ansibot added the support:core label Nov 5, 2018

@bcoca bcoca requested a review from abadger Nov 5, 2018

@bcoca bcoca removed the needs_triage label Nov 5, 2018

@ansibot ansibot added core_review and removed needs_revision labels Nov 5, 2018

@ansibot ansibot added the stale_ci label Nov 13, 2018

return distribution_version
def _get_all_subclasses(cls):

This comment has been minimized.

@abadger

abadger Dec 5, 2018

Member

This should probably become a general purpose, public utility function somewhere. If we move it here and make it private for the time being we should at least add a comment to show that we intend to move it somewhere permanent and make it public later.

This comment has been minimized.

@acalm

acalm Dec 5, 2018

Contributor

seems unnecessary to make it private if it's going to be public, _get_all_subclasses -> get_all_subclasses

This comment has been minimized.

@abadger

abadger Dec 5, 2018

Member

Well, the reason it would be private is that this isn't the location we want people to find it. It's not related to sys_info. It's only used by sys_info functions.

@abadger

This comment has been minimized.

Member

abadger commented Dec 5, 2018

Initial look, this looks good. I'll have @sdoran look as well.

def _get_all_subclasses(cls):
'''
used by modules like Hardware or Network fact classes to retrieve all subclasses of a given class.
__subclasses__ return only direct sub classes. This one go down into the class tree.

This comment has been minimized.

@samdoran

samdoran Dec 5, 2018

Member

Not sure what "This one go down into the class tree" means. I know it's from the original method, but see if you can improve this language so it's more clear.

This comment has been minimized.

@acalm

acalm Dec 5, 2018

Contributor

Not sure I managed to make it clearer, but there was an attempt :)

serole=dict(),
selevel=dict(),
setype=dict(),
attributes=dict(aliases=['attr'])

This comment has been minimized.

@samdoran

samdoran Dec 5, 2018

Member

Minor style change: add a , at the end of this line.

This comment has been minimized.

@abadger

abadger Dec 5, 2018

Member

We can also change dict() => {}.

@samdoran

Looks good. Only a few minor things.

@ansibot ansibot removed the stale_ci label Dec 5, 2018

acalm
@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 5, 2018

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

lib/ansible/module_utils/common/sys_info.py:56:85: trailing-whitespace Trailing whitespace

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

lib/ansible/module_utils/common/sys_info.py:56:86: W291 trailing whitespace

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Dec 5, 2018

acalm

@acalm acalm closed this Dec 7, 2018

@acalm acalm reopened this Dec 7, 2018

@abadger

This comment has been minimized.

Member

abadger commented Dec 7, 2018

rebuild_merge

@abadger abadger merged commit 876b637 into ansible:devel Dec 7, 2018

1 check failed

Shippable Run 97219 status is UNSTABLE.
Details
@abadger

This comment has been minimized.

Member

abadger commented Dec 7, 2018

Merged for 2.8

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