copr / copr

Clone

#1069 frontend: python3-flask-script removal
Merged 4 years ago by frostyx. Opened 4 years ago by thrnciar.
copr/ thrnciar/copr python3-flask-script-removal-test  into  master

file modified
+2 
@@ -69,6 +69,7 @@ 
 

  BuildRequires: fedora-messaging
 

  BuildRequires: libmodulemd < 2
 

  BuildRequires: libmodulemd >= 1.7.0
 

+ BuildRequires: python3-click
 

  BuildRequires: python3-CommonMark
 

  BuildRequires: python3-blinker
 

  BuildRequires: python3-copr-common > 0.4
 
@@ -120,6 +121,7 @@ 
 

  Requires: js-respond
 

  Requires: libmodulemd < 2
 

  Requires: libmodulemd >= 1.7.0
 

+ Requires: python3-click
 

  Requires: python3-CommonMark
 

  Requires: python3-alembic
 

  Requires: python3-blinker

file modified
+28 -34 
@@ -1,37 +1,31 @@ 
 

  import argparse
 

  import os
 

  import subprocess
 

- from flask_script import Command, Option
 

- 

- 

- class TestCommand(Command):
 

- 

-     def run(self, coverage, test_args):
 

-         os.environ["COPRS_ENVIRON_UNITTEST"] = "1"
 

-         if not (("COPR_CONFIG" in os.environ) and os.environ["COPR_CONFIG"]):
 

-             os.environ["COPR_CONFIG"] = "/etc/copr/copr_unit_test.conf"
 

- 

-         if 'PYTHONPATH' in os.environ:
 

-             os.environ['PYTHONPATH'] = os.environ['PYTHONPATH'] + ':.'
 

-         else:
 

-             os.environ['PYTHONPATH'] = '.'
 

- 

-         additional_args = test_args
 

- 

-         if coverage:
 

-             additional_args.extend([
 

-                 '--cov-report', 'term-missing', '--cov', 'coprs'
 

-             ])
 

- 

-         return subprocess.call(["/usr/bin/python3", "-m", "pytest"] + additional_args)
 

- 

-     option_list = (
 

-         Option("-a",
 

-                dest="test_args",
 

-                nargs=argparse.REMAINDER),
 

-         Option("--coverage",
 

-                dest="coverage",
 

-                required=False,
 

-                action='store_true',
 

-                default=False),
 

-     )
 

+ import click
 

+ 

+ @click.command(context_settings=dict(
 

+     ignore_unknown_options=True,
 

+ ))
 

+ @click.argument("arguments", nargs=-1, type=click.UNPROCESSED)
 

+ @click.option("--coverage/--no-coverage",
 

+     default=False
 

+ )
 

+ 

+ def test(coverage, arguments):
 

+     os.environ["COPRS_ENVIRON_UNITTEST"] = "1"
 

+     if not (("COPR_CONFIG" in os.environ) and os.environ["COPR_CONFIG"]):
 

+         os.environ["COPR_CONFIG"] = "/etc/copr/copr_unit_test.conf"
 

+ 

+     if 'PYTHONPATH' in os.environ:
 

+         os.environ['PYTHONPATH'] = os.environ['PYTHONPATH'] + ':.'
 

+     else:
 

+         os.environ['PYTHONPATH'] = '.'
 

+ 

+     additional_args = list(arguments)
 

+ 

+     if coverage:
 

+         additional_args.extend([
 

+             '--cov-report', 'term-missing', '--cov', 'coprs'
 

+         ])
 

+ 

+     return subprocess.call(["/usr/bin/python3", "-m", "pytest"] + additional_args)

file modified
+13 -6 
@@ -5,14 +5,14 @@ 
 

  import sys
 

  import pipes
 

  import importlib
 

+ import click
 

+ import commands.test
 

+ 

  from flask_script import Manager
 

  from coprs import app
 

  

  

- commands = {
 

-     # General commands
 

-     "test": "TestCommand",
 

- 

+ commands_old = {
 

      # Database commands
 

      "create_sqlite_file": "CreateSqliteFileCommand",
 

      "create_db": "CreateDBCommand",
 
@@ -57,11 +57,18 @@ 
 

      sys.exit(1)
 

  

  manager = Manager(app)
 

- for cmdname, clsname in commands.items():
 

+ for cmdname, clsname in commands_old.items():
 

      module = importlib.import_module("commands.{0}".format(cmdname))
 

      cls = getattr(module, clsname)
 

      manager.add_command(cmdname, cls())
 

  

+ app.cli.add_command(commands.test.test, "test")
 

  

  if __name__ == "__main__":
 

-     manager.run()
 

+     # This is just temporary while migrating to flask script,
 

+     # values in arrays are already migrated parameters.
 

+     # Else part will be removed once migration is complete.
 

+     if sys.argv[1] in ['test']:
 

+         app.cli()
 

+     else:
 

+         manager.run()

file modified
+1 -1 
@@ -18,4 +18,4 @@ 
 

  export COPR_CONFIG="$(pwd)/coprs_frontend/config/copr_unit_test.conf"
 

  

  cd coprs_frontend
 

- ./manage.py test -a "$@"
 

+ ./manage.py test "$@"

commands/test.py - done

rebased onto 6940aac1b20d250a664d5335df7e8ce1a94553b2
4 years ago

rebased onto c108d3c16a442e6f3fa5d5e84c7a77264d7d0887
4 years ago

LGTM +1

What is the purpose of this blank function?

Just a minor thing, but personally, I would go with import commands and then use commands.test.test later in the code. This way you don't have to care if two functions have the same name (which I don't know whether it may happen or not).

I would pick some order of those decorators and stick to it. @click.command() should go first, according to the documentation, then I would go with @click.argument because for any command, there will be only a small number of them. After that, I would put all the @click.option parameters.

Also, the -a and a is not a particularly good name :-). Previously, there was only -a and --coverage, so I am not sure what the positional a argument is for.

Thank you @thrnciar, the PR IMHO looks really good.
I made you a few notes in the code section.

Also pt2, the --help for -a is not very helpful

Options:
  -a TEXT...

can you please add some short description?

@frostyx, thank you for your feedback. I will try to explain it.

Also, the -a and a is not a particularly good name :-). Previously, there was only -a and --coverage, so I am not sure what the positional a argument is for.

The reason why the a is there is because I was trying to achieve unlimited number of arguments following the -a option. The problem was that it wasn't as easy as I firstly expected and I had to search for solution online. I have found this answer[1], that helped me to solve this problem. The reason why in my code is -a / a is because in his aswer he had --users / users. I can understand it's bit confusing. It would be probably better to ask, you guys, what was originally -a meant to be and give it some more descriptive name. So, do you remember for what is the -a option used?

[1] https://stackoverflow.com/a/54764354

rebased onto 0dbdf2c9a369a896a188dcad71d0d90a339f37ff
4 years ago

Do we need the multiple-groups feature? If not, we can simply discard this code and use:

app.cli.add_command(test, "test")

and then later replace manage_new() with app.cli()

I mean commands.test.test ^^

@thrnciar, does the following command work for you?

[jkadlcik@localhost frontend]$ ./run_tests.sh -k test_mail.py

I get Error: no such option: -k, which is wrong, but I am not sure whether it is PEBKAC from me.

@frostyx, are you sure, that you have test_mail.py in the same directory as ./run_tests.sh?
Shouldn't you run it as ./run_tests.sh -k coprs_frontend/tests/test_mail.py?

thanks, this is better. I'll update PR.

rebased onto a7498c88f97fc1b328328cb9ad34d8a6f9749a1c
4 years ago

rebased onto b9c0a5c89f7de9e1384ac1a3deab2ad5e1ec680a
4 years ago

rebased onto dacee14
4 years ago

rebased onto dacee14
4 years ago

+1

Commit cf4cfc3 fixes this pull-request

Pull-Request has been merged by frostyx
4 years ago
Changes Summary 4
+2 -0
file changed
frontend/copr-frontend.spec
+28 -34
file changed
frontend/coprs_frontend/commands/test.py
+13 -6
file changed
frontend/coprs_frontend/manage.py
+1 -1
file changed
frontend/run_tests.sh
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.