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.
--
Mailing list: https://launchpad.net/~canonical-ubuntu-qa
Post to : canonical-ubuntu-qa@lists.launchpad.net
Unsubscribe : https://launchpad.net/~canonical-ubuntu-qa
More help : https://help.launchpad.net/ListHelp
