canonical-ubuntu-qa team mailing list archive

[Brian Murray <mp+443359@xxxxxxxxxxxxxxxxxx>]

Review: Needs Information

Wow, this was a lot of work - thanks for doing it! It's also a lot of work to review and I don't think its realistic to review every single line.

Have you done any testing of the changed scripts? I wouldn't want to find out scripts we need to use for opening the archive are broken when we actually need to do it.  It's fine if you haven't tested them but I think we should come up with a test plan before actually merging this.

I also think it would be helpful to document why you decided to disable some of the shellcheck tests or pylint tests.

I've also a few in-line comments.

Diff comments:

> diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/cleanup-lxd b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/cleanup-lxd
> index 9b3d376..99a47f7 100755
> --- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/cleanup-lxd
> +++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/cleanup-lxd
> @@ -1,4 +1,8 @@
>  #!/usr/bin/python3
> +'''
> +Cleans up old lxd containers in autopkgtest-cloud
> +'''
> +#pylint: disable=invalid-name

In another bit of this MP you used "# pylint". Could you make the usage of it consistent?

>  import glob
>  import json
>  import os
> @@ -10,10 +14,12 @@ MINIMUM_AGE_MINS = 60
>  
>  
>  def parse_lxd_time(s):
> +    '''Get the age of the lxd container'''
>      return datetime.datetime.fromisoformat(s.split(".")[0] + "+00:00")
>  
>  
>  def check_remote(remote):
> +    '''Deletes containers that are too old'''

This isn't necessary to fix now but if the docstring is correct we should rename the function.

>      now = datetime.datetime.now(datetime.timezone.utc)
>      containers = json.loads(
>          subprocess.check_output(["lxc", "list", "-fjson", remote + ":"])
> diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
> index 55b8ef6..762f0c3 100755
> --- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
> +++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
> @@ -89,7 +91,8 @@ TEMPORARY_TEST_FAIL_STRINGS = ['Could not connect to ftpmaster.internal:80',
>                                 'Cannot initiate the connection to ppa.launchpad.net:80',
>                                 'Failed to fetch http://ftpmaster.internal/',
>                                 '" failed with stderr "error: Get https://0.0.0.0/1.0/operations/',
> -                               'RecursionError: maximum recursion depth exceeded in comparison',  # #1908506
> +                               'RecursionError: maximum recursion ' + \
> +                               'depth exceeded in comparison',  # #1908506

Let's make this "LP: #" so it will be linkifed by some terminal emulators.

>                                 'Temporary failure resolving \'archive.ubuntu.com\'',
>                                 'Temporary failure resolving \'ports.ubuntu.com\'',
>                                 'Temporary failure resolving \'ftpmaster.internal\'',
> @@ -406,18 +424,18 @@ def send_status_info(queue, release, architecture, pkgname, params, out_dir, run
>                        'logtail': logtail})
>      queue.basic_publish(amqp.Message(msg, delivery_mode=2), status_exchange_name, '')
>  
> -def call_autopkgtest(argv, release, architecture, pkgname, params, out_dir, start_time, private=False):
> +def call_autopkgtest(argv, release, architecture, pkgname,
> +                     params, out_dir, start_time, private=False):
>      '''Call autopkgtest and regularly send status/logtail to status_exchange_name

This seems inconsistent with the changes to "def send_status_info()" where each parameter is on a separate line, while here it looks like there are more parameters until we reach a certain line length. While I prefer the latter - I'd really just like things to be consistent.

>  
>      Return exit code.
>      '''
>      # set up status AMQP exchange
> -    global amqp_con
>      status_amqp = amqp_con.channel()
>      status_amqp.access_request('/data', active=True, read=False, write=True)
>      status_amqp.exchange_declare(status_exchange_name, 'fanout', durable=False, auto_delete=True)
>  
> -    null_fd = open('/dev/null', 'w')
> +    null_fd = open('/dev/null', 'w', encoding='utf-8')
>      autopkgtest = subprocess.Popen(argv, stdout=null_fd, stderr=subprocess.STDOUT)
>      # FIXME: Use autopkgtest.wait(timeout=10) once moving to Python 3
>      # only send status update every 10s, but check if program has finished every 1s
> @@ -978,20 +1014,19 @@ def request(msg):
>      finally:
>          shutil.rmtree(work_dir)
>  
> -    global amqp_con

Where did this go?

>      complete_amqp = amqp_con.channel()
>      complete_amqp.access_request('/complete', active=True, read=False, write=True)
>      complete_amqp.exchange_declare(complete_exchange_name, 'fanout', durable=True, auto_delete=False)
> -    complete_msg = json.dumps ({'architecture': architecture,
> -                                'container': container,
> -                                'duration': duration,
> -                                'exitcode': code,
> -                                'package': pkgname,
> -                                'testpkg_version': testpkg_version,
> -                                'release': release,
> -                                'requester': requester,
> -                                'swift_dir': swift_dir,
> -                                'triggers': triggers})
> +    complete_msg = json.dumps({'architecture': architecture,
> +                               'container': container,
> +                               'duration': duration,
> +                               'exitcode': code,
> +                               'package': pkgname,
> +                               'testpkg_version': testpkg_version,
> +                               'release': release,
> +                               'requester': requester,
> +                               'swift_dir': swift_dir,
> +                               'triggers': triggers})
>      complete_amqp.basic_publish(amqp.Message(complete_msg, delivery_mode=2),
>                                  complete_exchange_name, '')
>  
> diff --git a/ci/lint_test b/ci/lint_test
> index e52edc4..06a4133 100755
> --- a/ci/lint_test
> +++ b/ci/lint_test
> @@ -44,42 +44,56 @@ def remove_list_from_list(input_list, remove_list):
>      '''
>      Removes elements from remove_list from input_list
>      '''
> -    for ff in input_list:
> -        if os.path.isfile(ff):
> -            if str(ff) in remove_list:
> -                input_list.remove(ff)
> +    for list_elem in input_list:
> +        if os.path.isfile(list_elem):
> +            if str(list_elem) in remove_list:
> +                input_list.remove(list_elem)
>      return input_list
>  
>  
>  def run_lint_command(files_to_lint, lint_command, arguments=None):
>      '''
> -    Runs a given lint command over a list of filepaths and stores output
> +    Runs given lint commands over a list of filepaths and stores output
>      and exit code
>      '''
> -    exit_codes = 0
> -    lint_output = ""
> -    # check lint command exists
> -    for f in files_to_lint:
> -        if arguments is None:
> -            cmd = [lint_command, f]
> -            result = subprocess.run(cmd, stdout=subprocess.PIPE)
> -        else:
> -            cmd = [lint_command]
> -            for arg in arguments.split(" "):
> -                cmd.append(arg)
> -            cmd.append(f)
> -            result = subprocess.run(cmd, stdout=subprocess.PIPE)
> -        lint_output += result.stdout.decode("utf-8") + "\n"
> -        exit_codes += result.returncode
> -    return lint_output, exit_codes
> +    exit_codes = []
> +    lint_output = []
> +    lint_success = True
> +    check_for_cmd = subprocess.run(["which", lint_command], stdout=subprocess.PIPE, check=False)
> +    if check_for_cmd.returncode != 0:
> +        logger.error("%s not present on system - please amend before using this script.",
> +                     lint_command)
> +        sys.exit(1)
> +    for file in files_to_lint:
> +        if ".git" not in file:
> +            if arguments is None:
> +                cmd = [lint_command, file]
> +                result = subprocess.run(cmd, stdout=subprocess.PIPE, check=False)
> +            else:
> +                cmd = [lint_command]
> +                for arg in arguments.split(" "):
> +                    cmd.append(arg)
> +                cmd.append(file)
> +                result = subprocess.run(cmd, stdout=subprocess.PIPE, check=False)
> +            lint_output.append(result.stdout.decode("utf-8") + "\n")
> +            exit_codes.append(result.returncode)
> +            if result.returncode != 0:
> +                lint_success = False
> +    return lint_output, exit_codes, lint_success
>  
>  
> -if __name__=="__main__":
> +if __name__ == "__main__":
> +    # pylint: disable=invalid-name
> +    parser = argparse.ArgumentParser(description="Args for lint test")
> +    parser.add_argument('-v',
> +                        '--verbose',
> +                        help="Verbose output from lint test (y/n)",

Why does the help have "(y/n)" when the --verbose switch is a binary one?

> +                        action='store_true')
> +    args = parser.parse_args()
>      logging.basicConfig(level=logging.INFO)
>      logger = logging.getLogger('autopkgtest-cloud-linter')
>  
> -    start_dir = "../"
> -    repo_dir = pathlib.Path(start_dir)
> +    repo_dir = pathlib.Path("../")
>      repo_dir.rglob("*")
>  
>      final_list_of_python_files = []


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/443359
Your team Canonical's Ubuntu QA is subscribed to branch autopkgtest-cloud:master.