canonical-ubuntu-qa team mailing list archive

[Skia <mp+459166@xxxxxxxxxxxxxxxxxx>]

I don't really know that code or its purpose, but I've put you an inline comment anyway.

Diff comments:

> diff --git a/charms/focal/autopkgtest-web/webcontrol/update-github-jobs b/charms/focal/autopkgtest-web/webcontrol/update-github-jobs
> index 6362803..8c625ad 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/update-github-jobs
> +++ b/charms/focal/autopkgtest-web/webcontrol/update-github-jobs
> @@ -96,6 +98,20 @@ def finish_job(jobfile, params, code, log_url):
>      os.unlink(jobfile)
>  
>  
> +def check_time_diff(object_time, job_time):
> +    splitted_url = object_time.split("/")
> +    # date and time of object embedded in swift object path
> +    timestamp = splitted_url[4]
> +    # last part of the timestamp is a storage ID which we don't need
> +    timestamp = "_".join(timestamp.split("_")[0:2])
> +    timestamp = datetime.datetime.strptime(timestamp, "%Y%m%d_%H%M%S")
> +    diff = abs(timestamp.timestamp() - job_time) / DAY_IN_SECONDS
> +    # 15 days either side
> +    if diff < 15:

I don't mind hardcoding some values from time to time, but two things here:
* it's usually better to put that as a global var with good naming, a bit like you did with DAY_IN_SECONDS
* putting that test directly in that function seems odd to me: I would rather expect a more generic function returning the time difference, and then doing the comparison when calling that function. You could do for example `if get_absolute_time_diff(...) > MAX_DAY_DIFF: continue`

> +        return True
> +    return False
> +
> +
>  def process_job(jobfile):
>      try:
>          with open(jobfile) as f:


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/459166
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:fix-update-github-jobs into autopkgtest-cloud:master.