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

Add os-release file parsing, partially fix #25897 #33817

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
7 participants
@mscherer
Contributor

mscherer commented Dec 12, 2017

SUMMARY

Add os-release file parsing, partially fix #25897
/etc/os_release is documented on https://www.freedesktop.org/software/systemd/man/os-release.html

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

setup

ANSIBLE VERSION
$ ansible --version
ansible 2.5.0 (add_fact_os_release 1b2081dfbb) last updated 2017/12/12 15:56:51 (GMT +200)
  config file = /home/misc/.ansible.cfg
  configured module search path = [u'/home/misc/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/misc/checkout/git/ansible/lib/ansible
  executable location = /home/misc/checkout/git/ansible/bin/ansible
  python version = 2.7.5 (default, May  3 2017, 07:55:04) [GCC 4.8.5 20150623 (Red Hat 4.8.5-14)]
@mscherer

This comment has been minimized.

Contributor

mscherer commented Dec 12, 2017

I didn't took a look at testing that yet, but plan to add it to the unit tests.

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 12, 2017

@ansibot ansibot added the test label Dec 12, 2017

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch 5 times, most recently Dec 12, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 12, 2017

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

lib/ansible/module_utils/facts/system/os_release.py:37:19: E231 missing whitespace after ','
lib/ansible/module_utils/facts/system/os_release.py:51:1: W391 blank line at end of file

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

lib/ansible/module_utils/facts/system/os_release.py:51:0: trailing-newlines Trailing newlines

click here for bot help

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 12, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 12, 2017

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

lib/ansible/module_utils/facts/system/os_release.py:37:19: E231 missing whitespace after ','
lib/ansible/module_utils/facts/system/os_release.py:51:1: W391 blank line at end of file

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

lib/ansible/module_utils/facts/system/os_release.py:51:0: trailing-newlines Trailing newlines

click here for bot help

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch 3 times, most recently Dec 13, 2017

@ansibot ansibot removed the needs_revision label Dec 13, 2017

lib/ansible/module_utils/facts/system/os_release.py Outdated
def collect(self, module=None, collected_facts=None):
facts_dict = {}
path = None

This comment has been minimized.

@pilou-

pilou- Dec 13, 2017

Contributor

A comment about The file /etc/os-release takes precedence over /usr/lib/os-release. could be added there.

This comment has been minimized.

@mscherer

mscherer Dec 13, 2017

Contributor

Done

test/units/module_utils/facts/test_collectors.py Outdated
fact_namespace = 'ansible_os_release'
collector_class = OSReleaseFactCollector
@patch('ansible.module_utils.facts.system.os_release.get_file_content', return_value='# test\nID=clown_linux')

This comment has been minimized.

@pilou-

pilou- Dec 13, 2017

Contributor

Another variable with value using special characters outside of A–Z, a–z, 0–9 and enclosed with quotes might be added there.

This comment has been minimized.

@mscherer

mscherer Dec 13, 2017

Contributor

I added quotes, not sure about special char.

This comment has been minimized.

@pilou-

pilou- Dec 13, 2017

Contributor

A space is good enough :)

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 13, 2017

@pilou-

This comment has been minimized.

Contributor

pilou- commented Dec 13, 2017

Although it doesn't count: shipit .

@samdoran samdoran self-assigned this Dec 13, 2017

@gundalow gundalow requested a review from samdoran Dec 13, 2017

@gundalow gundalow removed the needs_triage label Dec 13, 2017

@alikins

I'd like to see the parse function be pulled out of the class. And for it to take a string for input and tests updated to use it.

lib/ansible/module_utils/facts/system/os_release.py Outdated
def parse_file(self, path):
result = {}
for line in get_file_content(path).splitlines():

This comment has been minimized.

@alikins

alikins Dec 13, 2017

Contributor

I would split this method into:

  • reading in the file content to a string
  • parsing the string

Then the test cases can test the parsing without having to patch get_file_content or any file io.

May be easier to test if the parsing function was module scope method as well, then the parsing tests dont
need to construct a collector or a faux AnsibleModule. The OsReleaseFactCollector would still be useful for
adding tests for existence of /etc/os-release and general class tests.

lib/ansible/module_utils/facts/system/os_release.py Outdated
for line in get_file_content(path).splitlines():
if line.startswith('#'):
continue
for t in shlex.split(line):

This comment has been minimized.

@alikins

alikins Dec 13, 2017

Contributor

One odd thing that has come up with the os-release parsing code in module_utils/facts/system/distribution.py is the quoting of some of the right hand side of lines.

I'll track down the bug momentarily, but at least one distro did odd things about what was/wasn't quoted (either VERSION wasnt quoted or ID was quoted?). The lsb/os-release spec is kind of vague on that, and different '/bin/lsb_release' tools seemed to do different things.

This isn't something that has to be addressed since I don't think there was a clear correct answer, but just mentioning it for context.

This comment has been minimized.

@mscherer

mscherer Dec 14, 2017

Contributor

So, while lsb is vague on that, I think, but the os-release specs is clear:
"Variable assignment values must be enclosed in double or single quotes if they include spaces, semicolons or other special characters outside of A–Z, a–z, 0–9. Shell special characters ("$", quotes, backslash, backtick) must be escaped with backslashes, following shell style."

Now of course, being on the right side of the specs do not help users :/

test/units/module_utils/facts/test_collectors.py Outdated
fact_namespace = 'ansible_os_release'
collector_class = OSReleaseFactCollector
@patch('ansible.module_utils.facts.system.os_release.get_file_content', return_value='# test\nID=clown_linux\nVERSION="Clown Linux"')

This comment has been minimized.

@alikins

alikins Dec 13, 2017

Contributor

I'd like to see more tests of the parsing, ideally with some examples of real os-release files. test/units/module_utils/test_distribtution.py[1] has some.

[1] test_distribution.py file is in the wrong place but nonetheless

@alikins

This comment has been minimized.

Contributor

alikins commented Dec 13, 2017

@mscherer #25725 and #31143 are the issue/pr related to the quoting in /etc/os-release I mentioned

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 14, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 14, 2017

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

test/units/module_utils/facts/test_os_release.py:57:70: SyntaxError: self.assertEqual(case['parsed_content'][k], result[k]

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

test/units/module_utils/facts/test_os_release.py:57:70: SyntaxError: self.assertEqual(case['parsed_content'][k], result[k]

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

test/units/module_utils/facts/test_os_release.py:57:70: SyntaxError: self.assertEqual(case['parsed_content'][k], result[k]

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

test/units/module_utils/facts/test_os_release.py:57:70: SyntaxError: self.assertEqual(case['parsed_content'][k], result[k]

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

test/units/module_utils/facts/test_os_release.py:57:70: SyntaxError: self.assertEqual(case['parsed_content'][k], result[k]

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

lib/ansible/module_utils/facts/system/os_release.py:26:1: E302 expected 2 blank lines, found 1
test/units/module_utils/facts/test_collectors.py:229:1: E302 expected 2 blank lines, found 1
test/units/module_utils/facts/test_os_release.py:58:1: E901 TokenError: EOF in multi-line statement

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

test/units/module_utils/facts/test_os_release.py:58:0: syntax-error unexpected EOF while parsing (<string>, line 58)

click here for bot help

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch 3 times, most recently Dec 14, 2017

@alikins

This comment has been minimized.

Contributor

alikins commented Dec 14, 2017

@alikins there is already a set of os-release file in test_distribution, so I either have to import that file in the test_os_release.py I am writing, or place the tests in test_distribution. Any preference ?

The 2nd do not look correct (different modules), the first mean that test have a dependencies, and I am not sure that's correct.

Or would it be better to just duplicate part of the data (I am leaning to that, since I also have to get the result). Would 1 file be enough, provided we test enough things ? (eg, a regular file, then corner case)

@mscherer I guess my preference would be to go ahead and mv test_distribution into test/units/module_utils/facts/system/test_distribution.py. Quick test that seems like just a 'git mv'

Then either:

  1. import test_distribution.TESTSETS into test_os_release
from . import test_distribution
   (or I guess, more ansible style would be)
from .test_distribution import TESTSETS
   but some variety of that...
  1. extract TESTSETS to a separate module or data file and import the testdata module into both

  2. A couple of unit tests setup some 'fixtures' data dirs we could try to emulate, but I don't think there is a consistent approach to that yet. Maybe something for the future...

Any of 1,2,3 would be okay with me. I wouldn't worry about trying #3 for this pr though.

@alikins

This comment has been minimized.

Contributor

alikins commented Dec 14, 2017

@mscherer I should mention the main advantage to the test_distributions.TESTSETS at the moment is that hacking/tests/gen_distribution_version_testcase.py can collect the needed sample info and dump it in the format used by TESTSETS.

That is a pretty simple script though, so nothing that can't be changed if you have a better idea on how to organize the test data.

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 14, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 14, 2017

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

test/units/module_utils/facts/test_os_release.py:51:29: undefined-variable Undefined variable 'unittest'

click here for bot help

@samdoran

This comment has been minimized.

Member

samdoran commented Dec 14, 2017

@mscherer Roger. We're trying to move these to BSD, but if we're stuck with GPL for this then so be it.

@mattclay

This comment has been minimized.

Member

mattclay commented Dec 15, 2017

CI failure in unit tests due to undefined variable caught by pylint.

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 15, 2017

@ansibot ansibot removed the ci_verified label Dec 15, 2017

@mscherer

This comment has been minimized.

Contributor

mscherer commented Dec 15, 2017

@alikins The problem is that TESTSETS do not contain the result of /etc/os_release parsing and if I have to generate it using the same code, I am not sure this is bringing much on the table vs hardcoding the file and the results in a separate data structure.

@mscherer mscherer force-pushed the mscherer:add_fact_os_release branch Dec 15, 2017

@samdoran samdoran removed their assignment Jan 30, 2018

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