PR#1602: mbs-manager uses click to replace Flask-Script - fm-orchestrator

fm-orchestrator

#1602 mbs-manager uses click to replace Flask-Script

Opened 7 months ago by cqi. Modified 7 months ago

cqi/fm-orchestrator drop-flask-script into master

mbs-manager uses click to replace Flask-Script

Chenxiong Qi • 7 months ago

b2198e3

module_build_service/manage.py

file modified

+57 -84

		`@@ -1,14 +1,13 @@`
		`# -- coding: utf-8 --`
		`# SPDX-License-Identifier: MIT`
		`from __future__ import absolute_import, print_function`
		`- from functools import wraps`
		`+ import click`
		`import getpass`
		`import logging`
		`import os`
		`- import textwrap`

		`import flask_migrate`
		`- from flask_script import Manager, prompt_bool`
		`+ from flask.cli import FlaskGroup`
		`from werkzeug.datastructures import FileStorage`

		`from module_build_service import app, db`
		`@@ -17,76 +16,39 @@`
		`)`
		`from module_build_service.common import conf, models`
		`from module_build_service.common.errors import StreamAmbigous`
		`- from module_build_service.common.logger import level_flags`
		`from module_build_service.common.utils import load_mmd_file, import_mmd`
		`import module_build_service.scheduler.consumer`
		`from module_build_service.scheduler.db_session import db_session`
		`import module_build_service.scheduler.local`
		`from module_build_service.web.submit import submit_module_build_from_yaml`

		`-`
		`- def create_app(debug=False, verbose=False, quiet=False):`
		`- # logging (intended for flask-script, see manage.py)`
		`- log = logging.getLogger(__name__)`
		`- if debug:`
		`- log.setLevel(level_flags["debug"])`
		`- elif verbose:`
		`- log.setLevel(level_flags["verbose"])`
		`- elif quiet:`
		`- log.setLevel(level_flags["quiet"])`
		`-`
		`- return app`
		`-`
		`-`
		`- manager = Manager(create_app)`
		`- help_args = ("-?", "--help")`
		`- manager.help_args = help_args`
		`- migrations_dir = os.path.join(os.path.abspath(os.path.dirname(__file__)),`
		`- 'migrations')`
		`+ migrations_dir = os.path.join(os.path.abspath(os.path.dirname(__file__)), "migrations")`
		`migrate = flask_migrate.Migrate(app, db, directory=migrations_dir)`
		`- manager.add_command("db", flask_migrate.MigrateCommand)`
		`- manager.add_option("-d", "--debug", dest="debug", action="store_true")`
		`- manager.add_option("-v", "--verbose", dest="verbose", action="store_true")`
		`- manager.add_option("-q", "--quiet", dest="quiet", action="store_true")`
		`-`
		`-`
		`- def console_script_help(f):`
		`- @wraps(f)`
		`- def wrapped(args, *kwargs):`
		`- import sys`

		`- if any([arg in help_args for arg in sys.argv[1:]]):`
		`- command = os.path.basename(sys.argv[0])`
		`- print(textwrap.dedent(`
		`- """\`
		`- {0}`

		`- Usage: {0} [{1}]`
		`+ @click.group(cls=FlaskGroup, create_app=lambda args, *kwargs: app)`
		`+ def cli():`
		`+ """MBS manager"""`

		`- See also:`
		`- mbs-manager(1)`
		`- """).strip().format(command, "\|".join(help_args))`
		`- )`
		`- sys.exit(2)`
		`- r = f(args, *kwargs)`
		`- return r`

		`- return wrapped`
		`+ cli.command("db", flask_migrate.MigrateCommand)`


		`- @console_script_help`
		`- @manager.command`
		`+ @cli.command("upgradedb")`
		`def upgradedb():`
		`""" Upgrades the database schema to the latest revision`
		`"""`
		`app.config["SERVER_NAME"] = "localhost"`
		`- # TODO: configurable?`
		`- migrations_dir = os.path.join(os.path.abspath(os.path.dirname(__file__)), "migrations")`
		`with app.app_context():`
		`flask_migrate.upgrade(directory=migrations_dir)`


		`- @manager.command`
		`+ def upgradedb_entrypoint():`
		`+ """Entrypoint for command mbs-upgradedb"""`
		`+ cli(["db", "upgrade"])`
		`+`
		`+`
		`+ @cli.command("cleardb")`
		`def cleardb():`
		`""" Clears the database`
		`"""`
		`@@ -94,8 +56,8 @@`
		`models.ComponentBuild.query.delete()`


		`- @console_script_help`
		`- @manager.command`
		`+ @cli.command("import_module")`
		`+ @click.argument("mmd_file", type=click.Path(exists=True))`
		`def import_module(mmd_file):`
		`""" Imports the module from mmd_file`
		`"""`
		`@@ -103,29 +65,44 @@`
		`import_mmd(db.session, mmd)`


		`- @manager.option("--stream", action="store", dest="stream")`
		`- @manager.option("--file", action="store", dest="yaml_file")`
		`- @manager.option("--srpm", action="append", default=[], dest="srpms", metavar="SRPM")`
		`- @manager.option("--skiptests", action="store_true", dest="skiptests")`
		`- @manager.option("--offline", action="store_true", dest="offline")`
		`- @manager.option("-d", "--debug", action="store_true", dest="log_debug")`
		`- @manager.option("-l", "--add-local-build", action="append", default=None, dest="local_build_nsvs")`
		`- @manager.option("-s", "--set-stream", action="append", default=[], dest="default_streams")`
		`- @manager.option(`
		`- "-r", "--platform-repo-file", action="append", default=[], dest="platform_repofiles"`
		`+ @cli.command("build_module_locally")`
		`+ @click.option("--stream", metavar="STREAM")`
		`+ @click.option(`
		`+ "--file", "yaml_file",`
		`+ metavar="FILE",`
		`+ required=True,`
		`+ type=click.Path(exists=True),`
		`+ )`
		`+ @click.option("--srpm", "srpms", metavar="SRPM", multiple=True)`
		`+ @click.option("--skiptests", is_flag=True)`
		`+ @click.option("--offline", is_flag=True)`
		`+ @click.option("-d", "--debug", "log_debug", is_flag=True)`
		`+ @click.option(`
		`+ "-l", "--add-local-build", "local_build_nsvs",`
		`+ metavar="NSV", multiple=True`
		`+ )`
		`+ @click.option(`
		`+ "-s", "--set-stream", "default_streams",`
		`+ metavar="STREAM", multiple=True`
		`+ )`
		`+ @click.option(`
		`+ "-r", "--platform-repo-file", "platform_repofiles",`
		`+ metavar="FILE",`
		`+ type=click.Path(exists=True),`
		`+ multiple=True`
		`)`
		`- @manager.option("-p", "--platform-id", action="store", default=None, dest="platform_id")`
		`+ @click.option("-p", "--platform-id", metavar="PLATFORM_ID")`
		`def build_module_locally(`
		`- local_build_nsvs=None,`
		`+ stream=None,`
		`yaml_file=None,`
		`srpms=None,`
		`- stream=None,`
		`skiptests=False,`
		`- default_streams=None,`
		`offline=False,`
		`+ log_debug=False,`
		`+ local_build_nsvs=None,`
		`+ default_streams=None,`
		`platform_repofiles=None,`
		`platform_id=None,`
		`- log_debug=False,`
		`):`
		`""" Performs local module build using Mock`
		`"""`
		`@@ -205,14 +182,11 @@`
		`raise RuntimeError("Module build failed")`


		`- @manager.option(`
		`- "identifier",`
		`- metavar="NAME:STREAM[:VERSION[:CONTEXT]]",`
		`- help="Identifier for selecting module builds to retire",`
		`- )`
		`- @manager.option(`
		`+ @cli.command("retire")`
		`+ @click.argument("identifier", metavar="NAME:STREAM[:VERSION[:CONTEXT]]")`
		`+ @click.option(`
		`"--confirm",`
		`- action="store_true",`
		`+ is_flag=True,`
		`default=False,`
		`help="Perform retire operation without prompting",`
		`)`
		`@@ -245,7 +219,8 @@`
		`logging.info("\t%s", ":".join((build.name, build.stream, build.version, build.context)))`

		`# Prompt for confirmation`
		`- is_confirmed = confirm or prompt_bool("Retire {} module builds?".format(len(module_builds)))`
		`+ confirm_msg = "Retire {} module builds?".format(len(module_builds))`
		`+ is_confirmed = confirm or click.confirm(confirm_msg, abort=False)`
		`if not is_confirmed:`
		`logging.info("Module builds were NOT retired.")`
		`return`
		`@@ -260,8 +235,10 @@`
		`logging.info("Module builds retired.")`


		`- @console_script_help`
		`- @manager.command`
		`+ @cli.command("run")`
		`+ @click.option("-h", "--host", metavar="HOST", help="Bind to this host.")`
		`+ @click.option("-p", "--port", metavar="PORT", help="Bind to this port along with --host.")`
		`+ @click.option("-d", "--debug", is_flag=True, default=False, help="Run frontend in debug mode.")`
		`def run(host=None, port=None, debug=None):`
		`""" Runs the Flask app, locally.`
		`"""`
		`@@ -274,9 +251,5 @@`
		`app.run(host=host, port=port, debug=debug)`


		`- def manager_wrapper():`
		`- manager.run()`
		`-`
		`-`
		`if __name__ == "__main__":`
		`- manager_wrapper()`
		`+ cli()`

requirements.txt

file modified

+1 -1

		`@@ -1,10 +1,10 @@`
		`+ click`
		`distro`
		`dogpile.cache`
		`enum34`
		`fedmsg`
		`Flask`
		`Flask-Migrate`
		`- Flask-Script`
		`Flask-SQLAlchemy`
		`funcsigs # Python2 only`
		`futures # Python 2 only`

setup.py

file modified

+2 -2

		`@@ -39,9 +39,9 @@`
		`dependency_links=deps_links,`
		`entry_points={`
		`"console_scripts": [`
		`- "mbs-upgradedb = module_build_service.manage:upgradedb",`
		`+ "mbs-upgradedb = module_build_service.manage:upgradedb_entrypoint",`
		`"mbs-frontend = module_build_service.manage:run",`
		`- "mbs-manager = module_build_service.manage:manager_wrapper",`
		`+ "mbs-manager = module_build_service.manage:cli",`
		`],`
		`"moksha.consumer": "mbsconsumer = module_build_service.scheduler.consumer:MBSConsumer",`
		`"mbs.messaging_backends": [`

tests/test_manage.py

file modified

+22 -16

		`@@ -5,10 +5,9 @@`
		`from mock import patch`
		`import pytest`

		`- from module_build_service import app`
		`+ from module_build_service import app, manage as mbs_manager`
		`from module_build_service.common import models`
		`from module_build_service.common.models import BUILD_STATES, ModuleBuild`
		`- from module_build_service.manage import manager_wrapper, retire`
		`from module_build_service.scheduler.db_session import db_session`
		`from module_build_service.web.utils import deps_to_dict`
		`from tests import clean_database, staged_data_filename`
		`@@ -30,10 +29,12 @@`
		`)`
		`def test_retire_identifier_validation(self, identifier, is_valid):`
		`if is_valid:`
		`- retire(identifier)`
		`+ with pytest.raises(SystemExit) as exc_info:`
		`+ mbs_manager.cli(["retire", identifier])`
		`+ assert 0 == exc_info`
		`else:`
		`with pytest.raises(ValueError):`
		`- retire(identifier)`
		`+ mbs_manager.cli(["retire", identifier])`

		`@pytest.mark.parametrize(`
		`("overrides", "identifier", "changed_count"),`
		`@@ -47,9 +48,9 @@`
		`({"context": "pickme"}, "spam:eggs:ham", 3),`
		`),`
		`)`
		`- @patch("module_build_service.manage.prompt_bool")`
		`- def test_retire_build(self, prompt_bool, overrides, identifier, changed_count):`
		`- prompt_bool.return_value = True`
		`+ @patch("click.confirm")`
		`+ def test_retire_build(self, confirm, overrides, identifier, changed_count):`
		`+ confirm.return_value = True`

		`module_builds = (`
		`db_session.query(ModuleBuild)`
		`@@ -71,7 +72,10 @@`

		`db_session.commit()`

		`- retire(identifier)`
		`+ with pytest.raises(SystemExit) as exc_info:`
		`+ mbs_manager.cli(["retire", identifier])`
		`+ assert 0 == exc_info.value`
		`+`
		`retired_module_builds = (`
		`db_session.query(ModuleBuild)`
		`.filter_by(state=BUILD_STATES["garbage"])`
		`@@ -93,11 +97,11 @@`
		`(False, True, True)`
		`),`
		`)`
		`- @patch("module_build_service.manage.prompt_bool")`
		`+ @patch("click.confirm")`
		`def test_retire_build_confirm_prompt(`
		`- self, prompt_bool, confirm_prompt, confirm_arg, confirm_expected`
		`+ self, confirm, confirm_prompt, confirm_arg, confirm_expected`
		`):`
		`- prompt_bool.return_value = confirm_prompt`
		`+ confirm.return_value = confirm_prompt`

		`module_builds = db_session.query(ModuleBuild).filter_by(state=BUILD_STATES["ready"]).all()`
		`# Verify our assumption of the amount of ModuleBuilds in database`
		`@@ -106,15 +110,17 @@`
		`for x, build in enumerate(module_builds):`
		`build.name = "spam" + str(x) if x > 0 else "spam"`
		`build.stream = "eggs"`
		`-`
		`db_session.commit()`

		`- retire("spam:eggs", confirm_arg)`
		`+ cmd = ["retire", "spam:eggs"] + (["--confirm"] if confirm_arg else [])`
		`+ with pytest.raises(SystemExit) as exc_info:`
		`+ mbs_manager.cli(cmd)`
		`+ assert 0 == exc_info.value`
		`+`
		`+ expected_changed_count = 1 if confirm_expected else 0`
		`retired_module_builds = (`
		`db_session.query(ModuleBuild).filter_by(state=BUILD_STATES["garbage"]).all()`
		`)`
		`-`
		`- expected_changed_count = 1 if confirm_expected else 0`
		`assert len(retired_module_builds) == expected_changed_count`


		`@@ -156,7 +162,7 @@`
		`original_db_uri = app.config["SQLALCHEMY_DATABASE_URI"]`
		`try:`
		`with patch("sys.argv", new=cli_cmd):`
		`- manager_wrapper()`
		`+ mbs_manager.cli()`
		`finally:`
		`app.config["SQLALCHEMY_DATABASE_URI"] = original_db_uri`

cqi commented 7 months ago

Resolves: FACTORY-4876

Signed-off-by: Chenxiong Qi cqi@redhat.com

cqi commented 7 months ago

There is a problem with mbs-upgradedb. When run this command, it reports following error:

Error: Could not locate a Flask application. You did not provide the "FLASK_APP" environment variable, and a "wsgi.py" or "app.py" module was not found in the current directory.

However, mbs-manager db upgrade works well. @mprahl Any idea?

mprahl commented on line 2 of requirements.txt 7 months ago

You will need to add click as a requirement since the version of Flask on RHEL 7 does not have click included.

pretty please pagure-ci rebuild

7 months ago

mprahl commented 7 months ago

There is a problem with mbs-upgradedb. When run this command, it reports following error:
Error: Could not locate a Flask application. You did not provide the "FLASK_APP" environment variable, and a "wsgi.py" or "app.py" module was not found in the current directory.
However, mbs-manager db upgrade works well. @mprahl Any idea?

I added raise ValueError("hello") to the upgradedb function.

Here is the traceback when run from mbs-manager upgradedb:

Traceback (most recent call last):
  File "/usr/bin/mbs-manager", line 9, in <module>
    load_entry_point('module-build-service==3.0.0', 'console_scripts', 'mbs-manager')()
  File "/usr/lib/python2.7/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/flask/cli.py", line 586, in main
    return super(FlaskGroup, self).main(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/lib/python2.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/flask/cli.py", line 426, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/src/module_build_service/manage.py", line 41, in upgradedb
    raise ValueError('hello')
ValueError: hello

Here is the traceback when run from mbs-upgradedb:

Traceback (most recent call last):
  File "/usr/bin/mbs-upgradedb", line 9, in <module>
    load_entry_point('module-build-service==3.0.0', 'console_scripts', 'mbs-upgradedb')()
  File "/usr/lib/python2.7/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/flask/cli.py", line 426, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/src/module_build_service/manage.py", line 41, in upgradedb
    raise ValueError('hello')

You can notice that the FlaskGroup is not instantiated when using mbs-upgradedb. So that might be a hint.

mprahl commented 7 months ago

@cqi please keep in mind that the version of Flask in RHEL 7 is 0.10.1. I don't think FlaskGroup exists in that version, but please confirm.

Our unit test container image based on CentOS 7 requires a newer version of Flask-SQLAlchemy which is installed through pip. I don't remember why, but this seems to also install a newer version of Flask, so you can't rely on the unit tests to test compatibility with that Flask version unfortunately.

rebased onto b2198e3

7 months ago

cqi commented 7 months ago

@mprahl Thanks for your review.

I write a simple wrapper for mbs-upgradedb as the entrypoint to workaround the problem.

I thought MBS runs containers based on fedora:31 when I saw Dockerfiles under openshift/. Flask 0.10.x in RHEL 7 does not have flask.cli. If this refactor is not required to be merge soon, it's ok for me to postpone the merge until MBS is deployed into an environment where flask.cli.FlaskGroup is available.

Edited 7 months ago by cqi

mprahl commented 7 months ago

@mprahl Thanks for your review.
I write a simple wrapper for mbs-upgradedb as the entrypoint to workaround the problem.
I thought MBS runs containers based on fedora:31 when I saw Dockerfiles under openshift/. Flask 0.10.x in RHEL 7 does not have flask.cli. If this refactor is not required to be merge soon, it's ok for me to postpone the merge until MBS is deployed into an environment where flask.cli.FlaskGroup is available.

@cqi the MBS deployments will not be containerized and will continue to run on RHEL 7 for the foreseeable future. This is because the new team managing MBS is more comfortable with that approach.

The openshift directory is for C3I only at the moment.

cqi commented 7 months ago

@mprahl Thanks for the info.

mprahl commented 7 months ago

@cqi what would you like to do with this PR? Are you planning to refactor it so it works on the version of Flask in EL7?