PR#32: Fix some flake8 errors - freshmaker

freshmaker

#32 Fix some flake8 errors

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

qwan/freshmaker fix-flake8-errors into master

Fix bandit issue: B108:hardcoded_tmp_directory

Qixiang Wan • 8 years ago

d34b500

Fix some flake8 errors

Qixiang Wan • 8 years ago

7f3571d

freshmaker/__init__.py

file modified

-2

		`@@ -40,5 +40,3 @@`
		`conf = init_config(app)`
		`init_logging(conf)`
		`log = getLogger(__name__)`
		`-`
		`- from freshmaker import views`

freshmaker/handlers/image_builder.py

file modified

+3 -3

		`@@ -56,9 +56,9 @@`

		`try:`
		`task_id = self.build_image(repo_url=event.repo_url,`
		`- rev=event.rev,`
		`- branch=event.branch,`
		`- namespace=event.namespace)`
		`+ rev=event.rev,`
		`+ branch=event.branch,`
		`+ namespace=event.namespace)`

		`self.record_build(event, event.repo, 'image', task_id)`

freshmaker/manage.py

file modified

+4 -6

		`@@ -26,11 +26,6 @@`
		`import os`
		`import ssl`

		`- import fedmsg.config`
		`- import moksha.hub`
		`- import moksha.hub.hub`
		`- import moksha.hub.reactor`
		`-`
		`from freshmaker import app, conf, db`
		`from freshmaker import models`

		`@@ -54,7 +49,7 @@`

		`See also:`
		`freshmaker-manager(1)""".format(command,`
		`- '\|'.join(help_args)))`
		`+ '\|'.join(help_args)))`
		`sys.exit(2)`
		`r = f(args, *kwargs)`
		`return r`
		`@@ -98,6 +93,7 @@`
		`with app.app_context():`
		`flask_migrate.upgrade(directory=migrations_dir)`

		`+`
		`@console_script_help`
		`@manager.command`
		`def cleardb():`
		`@@ -107,6 +103,7 @@`
		`models.ArtifactBuild.query.delete()`
		`db.session.commit()`

		`+`
		`@manager.command`
		`@console_script_help`
		`def generatelocalhostcert():`
		`@@ -162,5 +159,6 @@`
		`debug=debug`
		`)`

		`+`
		`if __name__ == "__main__":`
		`manager.run()`

freshmaker/models.py

file modified

+1 -1

		`@@ -47,7 +47,7 @@`
		`"rpm": 0,`
		`"image": 1,`
		`"module": 2,`
		`- }`
		`+ }`

		`INVERSE_ARTIFACT_TYPES = {v: k for k, v in ARTIFACT_TYPES.items()}`

freshmaker/utils.py

file modified

+4 -1

		`@@ -110,9 +110,12 @@`
		`return _run_command(cmd, rundir=repo, return_output=True).strip()`


		`- def _run_command(command, logger=None, rundir='/tmp', output=subprocess.PIPE, error=subprocess.PIPE, env=None, return_output=False):`
		`+ def _run_command(command, logger=None, rundir=None, output=subprocess.PIPE, error=subprocess.PIPE, env=None, return_output=False):`
		`"""Run a command, return output if return_output is True. Error out if command exit with non-zero code."""`

		`+ if rundir is None:`
		`+ rundir = tempfile.gettempdir()`
		`+`
		`if logger:`
		`logger.info("Running %s", subprocess.list2cmdline(command))`

freshmaker/views.py

file modified

+3 -4

		`@@ -21,12 +21,9 @@`
		`#`
		`# Written by Jan Kaluza <jkaluza@redhat.com>`

		`- import json`
		`- from flask import request, jsonify`
		`from flask.views import MethodView`

		`- from freshmaker import app, conf, log`
		`- from freshmaker import models, db`
		`+ from freshmaker import app`

		`api_v1 = {`
		`'freshmaker': {`
		`@@ -44,6 +41,7 @@`
		`def get(self, id):`
		`return "Done", 200`

		`+`
		`def register_api_v1():`
		`""" Registers version 1 of MBS API. """`
		`module_view = FreshmakerAPI.as_view('freshmaker')`
		`@@ -53,4 +51,5 @@`
		`view_func=module_view,`
		`**val['options'])`

		`+`
		`register_api_v1()`

tests/handlers/test_image_builder.py

file modified

+11 -12

		`@@ -245,18 +245,17 @@`
		`buildContainer = session.buildContainer`
		`self.assertEqual(2, buildContainer.call_count)`
		`buildContainer.assert_has_calls([`
		`- call('{}/container/{}?#{}'.format(conf.git_base_url,`
		`- 'testimage1',`
		`- last_commit_hash),`
		`- 'f25-container-candidate',`
		`- {'scratch': True, 'git_branch': 'f25'}),`
		`- call('{}/container/{}?#{}'.format(conf.git_base_url,`
		`- 'testimage2',`
		`- last_commit_hash),`
		`- 'f25-container-candidate',`
		`- {'scratch': True, 'git_branch': 'f25'}),`
		`- ],`
		`- any_order=True)`
		`+ call('{}/container/{}?#{}'.format(conf.git_base_url,`
		`+ 'testimage1',`
		`+ last_commit_hash),`
		`+ 'f25-container-candidate',`
		`+ {'scratch': True, 'git_branch': 'f25'}),`
		`+ call('{}/container/{}?#{}'.format(conf.git_base_url,`
		`+ 'testimage2',`
		`+ last_commit_hash),`
		`+ 'f25-container-candidate',`
		`+ {'scratch': True, 'git_branch': 'f25'}),`
		`+ ], any_order=True)`

		`events = models.Event.query.all()`
		`self.assertEquals(len(events), 1)`

tests/helpers.py

file modified

+9 -7

		`@@ -123,13 +123,15 @@`
		`self.repo = repo`
		`self.branch = branch`
		`self.rev = rev`
		`- self.stats = {'files': {},`
		`- 'total': {`
		`- 'additions': 0,`
		`- 'deletions': 0,`
		`- 'files': 0,`
		`- 'lines': 0,`
		`- }}`
		`+ self.stats = {`
		`+ 'files': {},`
		`+ 'total': {`
		`+ 'additions': 0,`
		`+ 'deletions': 0,`
		`+ 'files': 0,`
		`+ 'lines': 0,`
		`+ }`
		`+ }`

		`@property`
		`def inner_msg(self):`

tests/test_buildsys_handler.py

file modified

+2 -1

		`@@ -22,7 +22,7 @@`
		`import sys`
		`import unittest`

		`- sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`
		`+ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa`
		`from tests import helpers`

		`from freshmaker import events, db, models`
		`@@ -87,5 +87,6 @@`
		`build = models.ArtifactBuild.query.all()[0]`
		`self.assertEqual(build.state, models.BUILD_STATES['done'])`

		`+`
		`if __name__ == '__main__':`
		`unittest.main()`

tests/test_mbs_handler.py

file modified

+2 -1

		`@@ -23,7 +23,7 @@`
		`import unittest`
		`import mock`

		`- sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`
		`+ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa`
		`from tests import helpers`

		`from freshmaker import events, db, models`
		`@@ -373,5 +373,6 @@`
		`handler.handle(event)`
		`handler.rebuild_module.assert_not_called()`

		`+`
		`if __name__ == '__main__':`
		`unittest.main()`

tests/test_models.py

file modified

+2 -2

		`@@ -22,7 +22,7 @@`

		`import unittest`

		`- from freshmaker import conf, db`
		`+ from freshmaker import db`
		`from freshmaker.models import Event, ArtifactBuild`


		`@@ -41,7 +41,7 @@`
		`def test_creating_event_and_builds(self):`
		`event = Event.create(db.session, "test_msg_id")`
		`build = ArtifactBuild.create(db.session, event, "ed", "module", 1234)`
		`- build2 = ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build)`
		`+ ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build)`
		`db.session.commit()`
		`db.session.expire_all()`

no initial comment

cqi commented 8 years ago

:thumbsup:

sochotni commented 8 years ago

+1 - but do you already have things in place to make sure you don't diverge from standards again? Commit hooks and/or CI I mean

qwan commented 8 years ago

+1 - but do you already have things in place to make sure you don't diverge from standards again? Commit hooks and/or CI I mean

Not yet, usually I will run flake8 checking before push the commits, but not always. I'm not sure whether we can enable git hooks in central or push them to remote, probably not. It will just be an recommendation for devs to enable local pre-commit hook to run flake8 check, and for reviewers can run the checks too while reviewing a PR.

In #19, '--exit-zero' has been remove from the flake8 cmd, so I guess CI will enable this check after it merged. Seems something wrong with jenkins.fedorainfracloud.org at this moment, so can't check this.

cqi commented 8 years ago

ignore_outcome = True ignores the failure of flake8, when any issues detected by flake8, tox will not fail. To ensure not diverge from standards again, it looks that option should be removed as well.

1 new commit added

Fix bandit issue: B108:hardcoded_tmp_directory

8 years ago

qwan commented 8 years ago

ignore_outcome = True ignores the failure of flake8, when any issues detected by flake8, tox will not fail. To ensure not diverge from standards again, it looks that option should be removed as well.

Could you comment on #19 too? so we won't forget it.