#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

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()

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

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": [

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

  

Resolves: FACTORY-4876

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

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?

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

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.

@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

@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.

@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 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?