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

Lint Readable #238

Merged
merged 119 commits into from
Apr 2, 2020
Merged

Lint Readable #238

merged 119 commits into from
Apr 2, 2020

Conversation

GalRabin
Copy link
Contributor

@GalRabin GalRabin commented Mar 3, 2020

@anara123
@yaakovi
@yuvalbenshalom
@bakatzir

Status

In Progress

Related Issues

fixes: https://github.com/demisto/etc/issues/21345
fixes: https://github.com/demisto/etc/issues/22148
fixes: https://github.com/demisto/etc/issues/21750 @bakatzir
fixes: https://github.com/demisto/etc/issues/21569
reference: https://github.com/demisto/etc/issues/22759

Description

Lint command readable output.
Until today the lint only printed the raw output to the screen, due to that an overall optimization required in order to parse the data and supply a parsed output.

Changes:

  1. Logging option -v ... -vvvvvv (verbosity levels - logging levels verbose-critical- .. - debug)
  2. Logging path option - Until debug level.
  3. Docker-sdk, git-python usage for better exceptions handling and output,
  4. overall optimizations and code readability.
  5. JSON output including all collected data for analytics.
  6. Adding the following flags:
  • test-xml - export test results as xml.
  • json-report - export json report on all results.
  • log - save the log to file.
  1. Git python pack filtering - using set operations (save 19 sec- - each time).
  2. Facts gathering optimization - Pipfile collecting, modules from master, additional packages adding to each package 'test-requirements.txt` in the package.
  3. Context manager for mandatory test modules (CommonServerPython etc).
  4. Code readability.
  5. Docker image builds and not commit (Not the recommended way), Now we are using only Single Docker file to the setup test image.
  6. Adding many unit-tests in order to be more resilient, exceptions, fixtures etc - https://github.com/demisto/demisto-sdk/tree/lint/demisto_sdk/commands/lint/tests.

CI example

Change that should be done in config.yml in content repo:

demisto/content#6099

@GalRabin GalRabin requested a review from yaakovi March 3, 2020 20:05
@GalRabin GalRabin force-pushed the lint branch 6 times, most recently from 06b1a9e to 89d1d5e Compare March 9, 2020 14:02
@fvigo
Copy link

fvigo commented Mar 11, 2020

During the refactoring, please make sure that the error checks are in place. For example, running demisto-sdk lint on a machine that doesn't have docker returns this error, which is not very clear:

Traceback (most recent call last):
  File "/usr/local/bin/demisto-sdk", line 11, in <module>
    load_entry_point('demisto-sdk==0.4.2', 'console_scripts', 'demisto-sdk')()
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/click/decorators.py", line 73, in new_func
    return ctx.invoke(f, obj, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/demisto_sdk-0.4.2-py3.7.egg/demisto_sdk/__main__.py", line 321, in lint
    return linter.run_dev_packages()
  File "/usr/local/lib/python3.7/site-packages/demisto_sdk-0.4.2-py3.7.egg/demisto_sdk/commands/lint/lint_manager.py", line 126, in run_dev_packages
    run_status_code = linter.run_dev_packages()
  File "/usr/local/lib/python3.7/site-packages/demisto_sdk-0.4.2-py3.7.egg/demisto_sdk/commands/lint/linter.py", line 134, in run_dev_packages
    py_num = get_python_version(dockers[0], self.log_verbose)
  File "/usr/local/lib/python3.7/site-packages/demisto_sdk-0.4.2-py3.7.egg/demisto_sdk/commands/common/tools.py", line 453, in get_python_version
    universal_newlines=True, stderr=stderr_out).strip()
  File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 411, in check_output
    **kwargs).stdout
  File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 512, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['docker', 'run', '--rm', 'demisto/python3:3.7.4.2245', 'python', '-c', "import sys;print('{}.{}'.format(sys.version_info[0], sys.version_info[1]))"]' returned non-zero exit status 125.

@demisto demisto deleted a comment from lgtm-com bot Apr 1, 2020
@demisto demisto deleted a comment from lgtm-com bot Apr 1, 2020
@GalRabin
Copy link
Contributor Author

GalRabin commented Apr 1, 2020

Updating on changes done until now:

  1. Reducing the size of docker images (75% precent on alpine versions due to my implementation mistake)- Reduce the running time to 37:00 min nightly instead of 43:00 min
  2. Verbosity level by default on -vv (by @glicht request)
  3. Better user experience - tested on daily job of 12 developers, Recieved many feedbacks from the users regarding the output and I changed it to satisfied their needs.
  4. Bugs found in -
    • Powershell analyze and test has been fixed.
    • Git filters has been optimized and tested and diffrent enviorments.
    • Yaml filtering error fixed.

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2020

This pull request introduces 1 alert when merging 7de1f47 into 43ce5b0 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@GalRabin
Copy link
Contributor Author

GalRabin commented Apr 1, 2020

Copy link
Contributor

@glicht glicht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Happy to see this moving forward.

Please review unit test coverage as this PR decreases coverage.

demisto_sdk/__main__.py Outdated Show resolved Hide resolved
Co-Authored-By: Shai Yaakovi <30797606+yaakovi@users.noreply.github.com>
@GalRabin GalRabin removed the request for review from bakatzir April 2, 2020 08:11
@GalRabin GalRabin merged commit 127c740 into master Apr 2, 2020
@GalRabin GalRabin deleted the lint branch April 2, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants