-
Notifications
You must be signed in to change notification settings - Fork 685
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
Various python fixes #10345
Various python fixes #10345
Conversation
Hi @maage. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I think these are unrelated, issue probably was before, and change, if any, is not obvious |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, please rebase the MR to remove the merge commit.
Directory creation needs to be TOCTTOU resistant because tests etc can be parallel. Make mkdir_p return true if creation succeeded. If directory was not created, return false. In many cases checked existence of a entity (os.path.exists), not a directory. Error if target is not directory. On error raise.
When having '-' in '[...]' you need to put it first or last. [+-.] is actually '+', ',', '-', '.' 053 43 2B + 054 44 2C , 055 45 2D - 056 46 2E .
…ment From pyflakes: ssg/xml.py:431:5: redefinition of unused 'get_test_action_ref_element' from line 427
pyflakes: tests/ssg_test_suite/log.py:52:15: '...'.format(...) has unused arguments at position(s): 1 As format string does not have 2nd replacement, this must be mistake. See: https://docs.python.org/3/library/stdtypes.html#str.format
pyflakes: ssg/entities/profile_base.py:68:24: undefined name 'CPEDoesNotExist' ssg/entities/profile_base.py:72:27: undefined name 'CPEDoesNotExist'
pyflakes: ssg/content_diff.py:96:79: undefined name 'idref' ssg/content_diff.py:99:64: undefined name 'idref' ...
…ObjectException pyflakes: utils/ansible_playbook_to_role.py:436:19: undefined name 'UnknownObjectException'
At least: ./automatus.py rule <rule> ... Seems to work just fine with openscap-scanner v1.3.6.
pyflakes: tests/ssg_test_suite/common.py:435:13: '...'.format(...) has unused named argument(s): test_dir_config tests/test_profile_stability.py:140:13: '...'.format(...) has unused named argument(s): test_root tests/unit/ssg-module/test_id_translate.py:182:13: '...'.format(...) has unused named argument(s): ou
pyflakes: utils/srg_audit.py:100:26: f-string is missing placeholders utils/srg_audit.py:104:26: f-string is missing placeholders
pyflakes: tests/unit/ssg-module/test_build_remediations.py:71:5: local variable 'fixes' is assigned to but never used
flake8: utils/add_platform_rule.py:232:12: E711 comparison to None should be 'if cond is None:' utils/add_platform_rule.py:323:20: E711 comparison to None should be 'if cond is None:'
…o compare to truthy flake8: tests/unit/ssg_test_suite/test_matches_platform.py:9:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:15:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:21:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:27:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:33:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:40:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:47:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:54:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:60:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:66:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:72:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:79:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:86:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:93:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:101:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:108:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:116:72: E712 comparison to True should be 'if cond is True:' or 'if cond:' tests/unit/ssg_test_suite/test_matches_platform.py:123:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:' tests/unit/ssg_test_suite/test_matches_platform.py:131:72: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
flake8: build-scripts/verify_references.py:255:21: E713 test for membership should be 'not in' build-scripts/verify_references.py:260:21: E713 test for membership should be 'not in'
utils/mod_checks.py:159:8: E713 test for membership should be 'not in' utils/mod_fixes.py:163:8: E713 test for membership should be 'not in' utils/mod_prodtype.py:169:8: E713 test for membership should be 'not in'
Now should be rebased to fresh master w/o merge commits. |
Code Climate has analyzed commit d386ef3 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 43.4% (50% is the threshold). This pull request will bring the total coverage in the repository to 51.9% (0.1% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This should improve many things. I am waving the Code Climate findings as they are on existing code.
Description:
Collection of various python code fixes and some style changes.
Rationale:
Improve
mkdir_p
and then use it everywhere. There was duplicate implementations. Now also handle errors when directory already exists if there is issues with parallelism for example. Patch could further improved if only recent python versions are used.Fix
escape_regex
to match comment.Probably search and replace fix with xccdf version.
Some duplicate code. Some codes I wonder how it was previously working as intended.
Bad helper in automatus.
And then some style changes that I see as obvious.
Review Hints:
pyflakes / flake8 issues are only reduced before and after this patch. If you ignore file length.
Parallel builds to check
mkdir_p
before and after. It might be possible to effectively and repeatedly to see issues this might require cmake modifications to allow further parallelism.escape_regex
change might cause xml modifications.Sofar only built and run ctest with this.