PR#19: tox fixes - freshmaker

freshmaker

#19 tox fixes

Merged 8 years ago by ralph. Opened 8 years ago by fivaldi.

fivaldi/freshmaker fivaldi_ansible_tox into master

Filip Valder • 8 years ago

3ddcabf

tox.ini

file modified

+8 -20

		`@@ -7,8 +7,10 @@`
		`envlist = py27, py35, coverage, flake8, bandit`

		`[testenv]`
		`- deps = pytest`
		`+ # using sitepackages is not a good idea, but Koji... :(`
		`sitepackages = True`
		`+ install_command = pip install --force-reinstall --ignore-installed {packages}`
		`+ deps = pytest`
		`commands = py.test {posargs}`

		`[testenv:coverage]`
		`@@ -19,33 +21,19 @@`
		`commands =`
		`coverage run --parallel-mode -m pytest`
		`coverage combine`
		`- coverage report --include="freshmaker/.py,fedmsg.d/.py" --omit=.tox/* -m --skip-covered`
		`+ coverage report --omit=tests/,.tox/,/usr/* -m --skip-covered`

		`[testenv:flake8]`
		`basepython = python3`
		`skip_install = true`
		`deps = flake8`
		`- commands = flake8 --ignore E501,E731 --exit-zero`
		`+ commands = flake8 --ignore E501,E731`
qwan commented 8 years ago we can ignore files under freshmaker/migrations by excluding them here
		`+ ignore_outcome = True`

		`[testenv:bandit]`
		`basepython = python3`
		`skip_install = true`
		`deps = bandit`
		`commands =`
		`- /bin/bash -c "bandit -r $(find . -mindepth 1 -maxdepth 1 ! -name tests ! -name \.\* -type d -o -name \*.py) \|\| exit 0"`
		`-`
		`- [testenv:build]`
		`- basepython = python3`
		`- skip_install = true`
		`- deps = setuptools`
		`- commands = python setup.py sdist`
		`-`
		`- [testenv:release]`
		`- basepython = python3`
		`- skip_install = true`
		`- deps =`
		`- {[testenv:build]deps}`
		`- twine`
		`- commands =`
		`- {[testenv:build]commands}`
		`- twine upload --skip-existing dist/* {posargs}`
		`+ /bin/bash -c "bandit -r -ll $(find . -mindepth 1 -maxdepth 1 ! -name tests ! -name \.\* -type d -o -name \*.py)"`
		`+ ignore_outcome = True`

no initial comment

rebased

8 years ago

fivaldi commented 8 years ago

This PR is ready for review.

ralph commented 8 years ago

Running this with detox works for me.

bandit and flake8 both found lots of "problems".

Someone else should probably review this too though (one of @jkaluza, @cqi, or @qwan).

cqi commented 8 years ago

Works for me by running tox, but got warnings

coverage runtests: commands[1] | coverage combine
WARNING:test command found but not installed in testenv
  cmd: /usr/bin/coverage
  env: /home/cqi/code/factory2/code/freshmaker/.tox/coverage
Maybe you forgot to specify a dependency? See also the whitelist_externals envconfig setting.
coverage runtests: commands[2] | coverage report --omit=tests/*,.tox/*,/usr/* -m --skip-covered
WARNING:test command found but not installed in testenv
  cmd: /usr/bin/coverage
  env: /home/cqi/code/factory2/code/freshmaker/.tox/coverage
Maybe you forgot to specify a dependency? See also the whitelist_externals envconfig setting.

Relative to this change?

Suggest to merge this patch and use another PR to fix those "problems".

qwan commented 8 years ago

There are some flake8 errors I've been fixed in #32, for some other flake8 errors reported against migrations/*, we can ignore them since they're generated automatically.

For bandit reported errors, all are expected behavior can be ignored:
1. bind to 0.0.0.0 interface in config
2. Try-Except-Pass in config to override config_section
3. using subprocess risk
4. harded coded '/tmp' as default value of rundir in utils._run_command

For coverage warning, it's caused by sitepackages = True, but we need it due to 'koji'... so can ignore it too.

qwan commented on line 25 of tox.ini 8 years ago

we can ignore files under freshmaker/migrations by excluding them here

cqi commented 8 years ago

Try-Except-Pass in config to override config_section

Replace with ?

if os.path.exists(config_file) and os.access(config_file, os.R_OK):
    config_section = 'ProdConfiguration'

harded coded '/tmp' as default value of rundir in utils._run_command

/tmp could be replaced with tempfile.gettempdir()

qwan commented 8 years ago

Try-Except-Pass in config to override config_section

Replace with ?
if os.path.exists(config_file) and os.access(config_file, os.R_OK):
config_section = 'ProdConfiguration'

It is desirable to suppress exceptions here, so I don't think we need the change.

harded coded '/tmp' as default value of rundir in utils._run_command

/tmp could be replaced with tempfile.gettempdir()

Bandit reports this potential security issue, but actually not true in this case, I think we can ignore it too. Reference: https://security.openstack.org/guidelines/dg_using-temporary-files-securely.html

cqi commented 8 years ago

We can get the system temporary directory from Python standard library, why do we need to hard-coded it?

qwan commented 8 years ago