From afca7fbb192198140a3ea12bd5a74003024a19e2 Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 13:54:23 -0700 Subject: [PATCH 1/6] ENH: check github via ping before proceeding to avoid later long timeouts --- scripts/ioc_deploy.py | 55 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index 1cd71949..489d2ccf 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -219,6 +219,11 @@ def main_deploy(args: CliArgs) -> int: Will either return an int return code or raise. """ + logger.info("Checking github connectivity") + if not get_github_available(verbose=args.verbose): + logger.error("Github is not reachable, please check to make sure you're on a psbuild host.") + return ReturnCode.EXCEPTION + deploy_info = get_deploy_info(args) deploy_dir = deploy_info.deploy_dir pkg_name = deploy_info.pkg_name @@ -720,6 +725,56 @@ def _clone( return subprocess.run(cmd, **kwds) +def get_github_available(verbose: bool = False) -> bool: + """ + Return whether or not github is available. + """ + try: + _ping( + hostname="github.com", + count=1, + wait=1.0, + tries=3, + verbose=verbose, + ) + except subprocess.CalledProcessError: + return False + return True + + +def _ping( + hostname: str, + count: int, + wait: float, + tries: int = 1, + verbose: bool = False, +) -> subprocess.CompletedProcess: + """ + Ping a host some tries number of times, return the last completed process or raise. + """ + cmd = ["ping", "-c", str(count), "-W", str(wait), hostname] + if tries < 1: + tries = 1 + kwds = { + "check": True, + "universal_newlines": True, + } + if not verbose: + kwds["stdout"] = subprocess.PIPE + kwds["stderr"] = subprocess.PIPE + last_exc = None + for _ in range(tries): + try: + proc = subprocess.run(cmd, **kwds) + except subprocess.CalledProcessError as exc: + last_exc = exc + else: + return proc + if last_exc is None: + raise RuntimeError("Impossible code path?") + raise last_exc + + def print_help_text_for_readme(): """ Prints a text blob for me to paste into the release notes table. From 902972250326bdde94254246a552e782aed6e2a5 Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 13:54:59 -0700 Subject: [PATCH 2/6] NIT: remove duplicate text from log message --- scripts/ioc_deploy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index 489d2ccf..1aa416fc 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -839,7 +839,7 @@ def _main() -> int: if args.version: print(get_version()) return ReturnCode.SUCCESS - logger.info("ioc-deploy: checking inputs") + logger.info("Checking inputs") if not (args.name and args.release) and not args.path_override: logger.error( "Must provide both --name and --release, or --path-override. Check ioc-deploy --help for usage." From 5ed0f5399995dd447f457b572e2919d66ff3b4a6 Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 14:16:49 -0700 Subject: [PATCH 3/6] ENH: use git ls-remote instead of git clone to check tags --- scripts/ioc_deploy.py | 81 +++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index 1aa416fc..3fbb91df 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -529,25 +529,17 @@ def finalize_tag(name: str, github_org: str, release: str, verbose: bool) -> str - v1.0.0 - 1.0.0 """ - try_release = release_permutations(release=release) - - with TemporaryDirectory() as tmpdir: - for rel in try_release: - logger.debug(f"Checking for release {rel} in {github_org}/{name}") - try: - _clone( - name=name, - github_org=github_org, - release=rel, - working_dir=tmpdir, - target_dir=rel, - verbose=verbose, - ) - except subprocess.CalledProcessError: - logger.warning(f"Did not find release {rel} in {github_org}/{name}") - else: - logger.info(f"Release {rel} exists in {github_org}/{name}") - return rel + logger.debug(f"Getting all tags in {github_org}/{name}") + tags = get_repo_tags( + name=name, + github_org=github_org, + verbose=verbose, + ) + for rel in release_permutations(release=release): + logger.debug(f"Trying variant {rel}") + if rel in tags: + logger.info(f"Release {rel} exists in {github_org}/{name}") + return rel raise ValueError(f"Unable to find {release} in {github_org}/{name}") @@ -775,6 +767,57 @@ def _ping( raise last_exc +def get_repo_tags( + name: str, + github_org: str, + verbose: bool = False, +) -> List[str]: + """ + Get a list of tags that exist in the github repo. + + Raises a subprocess.CalledProcessError if the repo doesn't exist + or we have insufficient permissions. + """ + lines = _ls_remote(name=name, github_org=github_org, verbose=verbose) + tags = [] + for line in lines: + if "refs/tags/" not in line: + continue + tags.append(line.split("refs/tags/")[-1]) + return tags + + +def _ls_remote( + name: str, + github_org: str, + verbose: bool = False, +) -> List[str]: + """ + Run git ls-remote --tags --refs or raise a subprocess.CalledProcessError + + The code here is more complex than _clone so we can print stdout and stderr + interleaved in verbose mode while also capturing stdout separately. + This isn't possible with subprocess.run. + + Returns the stdout lines as a list of strings. + """ + cmd = ["git", "ls-remote", "--tags", "--refs", f"git@github.com:{github_org}/{name}"] + kwds = { + "stdout": subprocess.PIPE, + "bufsize": 1, + "universal_newlines": True, + } + if not verbose: + kwds["stderr"] = subprocess.PIPE + output = [] + with subprocess.Popen(cmd, **kwds) as proc: + for line in proc.stdout: + if verbose: + print(line, end="") + output.append(line.strip()) + return output + + def print_help_text_for_readme(): """ Prints a text blob for me to paste into the release notes table. From 8eaf77b51db98bacb48f5d3395eaa67babd9befb Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 15:26:20 -0700 Subject: [PATCH 4/6] REF: rearrange logic to decrease git clone count --- scripts/ioc_deploy.py | 178 +++++++++++++++++++++++++----------------- 1 file changed, 106 insertions(+), 72 deletions(-) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index 3fbb91df..ad6e6102 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -52,7 +52,7 @@ import sys from pathlib import Path from tempfile import TemporaryDirectory -from typing import List, Optional, Tuple +from typing import List, Tuple EPICS_SITE_TOP_DEFAULT = "/cds/group/pcds/epics" GITHUB_ORG_DEFAULT = "pcdshub" @@ -89,8 +89,8 @@ class DeployInfo: """ deploy_dir: str - pkg_name: Optional[str] - rel_name: Optional[str] + pkg_name: str + rel_name: str else: from types import SimpleNamespace @@ -224,14 +224,15 @@ def main_deploy(args: CliArgs) -> int: logger.error("Github is not reachable, please check to make sure you're on a psbuild host.") return ReturnCode.EXCEPTION + logger.info("Checking repos and ioc deploy directories") deploy_info = get_deploy_info(args) deploy_dir = deploy_info.deploy_dir pkg_name = deploy_info.pkg_name rel_name = deploy_info.rel_name - if pkg_name is None or rel_name is None: + if not (deploy_dir and pkg_name and rel_name): logger.error( - f"Something went wrong at package/tag normalization: package {pkg_name} at version {rel_name}" + f"Something went wrong at package/tag normalization: package {pkg_name} at version {rel_name} to dir {deploy_dir}" ) return ReturnCode.EXCEPTION @@ -325,36 +326,84 @@ def get_deploy_info(args: CliArgs) -> DeployInfo: Normalize user inputs and figure out where to deploy to. Returns the following in a dataclass: - - The deploy dir (deploy_dir) + - The directory name we'll deploy to (deploy_dir) - A normalized package name if applicable, or None (pkg_name) - A normalized tag name if applicable, or None (rel_name) + + Normalized names will have correct casing and be well-formed. + + A name is well-formed if it starts with "ioc", is hyphen-delimited, + and has at least three sections. + + For example, "ioc-common-gigECam" is a well-formed name for the purposes + of an IOC deployment. "ads-ioc" and "pcdsdevices" are not. + + However, "ads-ioc" will resolve to "ioc-common-ads-ioc". + Only common IOCs will be automatically discovered using this method. + + Normalized tag names will match existing repo tags if you forget or + pick the wrong tag prefix. + + This can go in one of three ways: + 1. start from a name and tag, get a path (--name, --release) + 2. start from a path, get a name and tag (--path-override) + 3. validate a name and tag, then use the path (all three args) """ - if args.name and args.github_org: - pkg_name = finalize_name( - name=args.name, + deploy_dir = "" + + if args.path_override: + deploy_dir = args.path_override + if (args.name and args.release): + name = args.name + release = args.release + else: + # Decompose /some/long/path/area/suffix/release + # or, maybe /some/long/path/area/suffix/release/ + dir, tail = os.path.split(deploy_dir) + if tail: + release = tail + else: + # One extra split if it ends with a / + dir, release = os.path.split(dir) + # Now it's /some/long/path/area/suffix + dir, suffix = os.path.split(dir) + _, area = os.path.split(dir) + name = args.name or "-".join(("ioc", area, suffix)) + release = args.release or release + else: + name = args.name + release = args.release + + # Force name into ioc-area-suffix structure before finalize_tag + if name and (len(name) < 5 or name[:4] != "ioc-"): + new_name = f"ioc-common-{name}" + logger.warning(f"{name} is not an ioc name, trying {new_name}") + name = new_name + + if name and release and args.github_org: + # Implicitly check if our repo exists here + # Let exceptions bubble up to _main + release = finalize_tag( + name=name, github_org=args.github_org, - ioc_dir=args.ioc_dir, + release=release, verbose=args.verbose, ) - else: - pkg_name = None - if pkg_name and args.github_org and args.release: - rel_name = finalize_tag( - name=pkg_name, + + if name: + name = finalize_name( + name=name, github_org=args.github_org, - release=args.release, + ioc_dir=args.ioc_dir, verbose=args.verbose, ) - else: - rel_name = None - if args.path_override: - deploy_dir = args.path_override - else: + + if not args.path_override: deploy_dir = get_target_dir( - name=pkg_name, ioc_dir=args.ioc_dir, release=rel_name + name=name, ioc_dir=args.ioc_dir, release=release ) - return DeployInfo(deploy_dir=deploy_dir, pkg_name=pkg_name, rel_name=rel_name) + return DeployInfo(deploy_dir=deploy_dir, pkg_name=name, rel_name=release) def get_perms_target(args: CliArgs) -> str: @@ -395,48 +444,8 @@ def find_casing_in_dir(dir: str, name: str) -> str: def finalize_name(name: str, github_org: str, ioc_dir: str, verbose: bool) -> str: """ - Check if name is present in org, is well-formed, and has correct casing. - - If the name is present, return it, fixing the casing if needed. - If the name is not present and the correct name can be guessed, guess. - If the name is not present and cannot be guessed, raise. - - A name is well-formed if it starts with "ioc", is hyphen-delimited, - and has at least three sections. - - For example, "ioc-common-gigECam" is a well-formed name for the purposes - of an IOC deployment. "ads-ioc" and "pcdsdevices" are not. - - However, "ads-ioc" will resolve to "ioc-common-ads-ioc". - Only common IOCs will be automatically discovered using this method. - - Note that GitHub URLs are case-insensitive, so there's no native way to tell - from a clone step if you've maintained the correct casing information. + Fix name's casing if necessary, checking existing deployments and github as needed. """ - split_name = name.split("-") - if len(split_name) < 3 or split_name[0] != "ioc": - new_name = f"ioc-common-{name}" - logger.warning(f"{name} is not an ioc name, trying {new_name}") - name = new_name - logger.debug(f"Checking for {name} in org {github_org}") - with TemporaryDirectory() as tmpdir: - try: - _clone( - name=name, github_org=github_org, working_dir=tmpdir, verbose=verbose - ) - except subprocess.CalledProcessError as exc: - raise ValueError( - f"Error cloning repo, make sure {name} exists in {github_org} and check your permissions!" - ) from exc - readme_text = "" - # Search for readme in any casing with any file extension - pattern = "".join(f"[{char.lower()}{char.upper()}]" for char in "readme") + "*" - for readme_path in (Path(tmpdir) / name).glob(pattern): - with open(readme_path, "r") as fd: - readme_text += fd.read() - logger.debug("Successfully read repo readme for backup casing check") - if not readme_text: - logger.debug("Unable to read repo readme for backup casing check") logger.debug("Checking deploy area for casing") # GitHub URLs are case-insensitive, so we need further checks # REST API is most reliable but requires different auth @@ -455,7 +464,7 @@ def finalize_name(name: str, github_org: str, ioc_dir: str, verbose: bool) -> st area = find_casing_in_dir(dir=ioc_dir, name=area) except RuntimeError: logger.info("This is a new area, checking readme for casing") - name = casing_from_readme(name=name, readme_text=readme_text) + name = casing_from_readme_clone(name=name, github_org=github_org, verbose=verbose) logger.info(f"Using casing: {name}") return name logger.info(f"Using {area} as the area") @@ -464,8 +473,8 @@ def finalize_name(name: str, github_org: str, ioc_dir: str, verbose: bool) -> st suffix = find_casing_in_dir(dir=str(Path(ioc_dir) / area), name=suffix) except RuntimeError: logger.info("This is a new ioc, checking readme for casing") + casing = casing_from_readme_clone(name=name, github_org=github_org, verbose=verbose) # Use suffix from readme but keep area from directory search - casing = casing_from_readme(name=name, readme_text=readme_text) suffix = split_ioc_name(casing)[2] logger.info(f"Using {suffix} as the name") @@ -481,7 +490,29 @@ def split_ioc_name(name: str) -> Tuple[str, str, str]: return tuple(name.split("-", maxsplit=2)) -def casing_from_readme(name: str, readme_text: str) -> str: +def casing_from_readme_clone(name: str, github_org: str, verbose: bool) -> str: + with TemporaryDirectory() as tmpdir: + try: + _clone( + name=name, github_org=github_org, working_dir=tmpdir, verbose=verbose + ) + except subprocess.CalledProcessError as exc: + raise ValueError( + f"Error cloning repo, make sure {name} exists in {github_org} and check your permissions!" + ) from exc + readme_text = "" + # Search for readme in any casing with any file extension + pattern = "".join(f"[{char.lower()}{char.upper()}]" for char in "readme") + "*" + for readme_path in (Path(tmpdir) / name).glob(pattern): + with open(readme_path, "r") as fd: + readme_text += fd.read() + logger.debug("Successfully read repo readme for casing check") + if not readme_text: + logger.debug("Unable to read repo readme for casing check") + return casing_from_readme_text(name=name, readme_text=readme_text) + + +def casing_from_readme_text(name: str, readme_text: str) -> str: """ Returns the correct casing of name in readme_text if available. @@ -530,11 +561,14 @@ def finalize_tag(name: str, github_org: str, release: str, verbose: bool) -> str - 1.0.0 """ logger.debug(f"Getting all tags in {github_org}/{name}") - tags = get_repo_tags( - name=name, - github_org=github_org, - verbose=verbose, - ) + try: + tags = get_repo_tags( + name=name, + github_org=github_org, + verbose=verbose, + ) + except subprocess.CalledProcessError as exc: + raise ValueError(f"Unable to access {github_org}/{name}") from exc for rel in release_permutations(release=release): logger.debug(f"Trying variant {rel}") if rel in tags: From 30a85d5746eec0df1402197ccc5675c0dbab5aba Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 15:40:36 -0700 Subject: [PATCH 5/6] ENH: nitpick ls-remote error handling and clean up user-facing messages. --- scripts/ioc_deploy.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index ad6e6102..85e3c161 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -568,7 +568,7 @@ def finalize_tag(name: str, github_org: str, release: str, verbose: bool) -> str verbose=verbose, ) except subprocess.CalledProcessError as exc: - raise ValueError(f"Unable to access {github_org}/{name}") from exc + raise ValueError(f"Unable to access {github_org}/{name}, please make sure you have the correct access rights and the repository exists.") from exc for rel in release_permutations(release=release): logger.debug(f"Trying variant {rel}") if rel in tags: @@ -827,7 +827,10 @@ def _ls_remote( verbose: bool = False, ) -> List[str]: """ - Run git ls-remote --tags --refs or raise a subprocess.CalledProcessError + Return git ls-remote's output or raise a subprocess.CalledProcessError + + This will call "git ls-remote --tags --refs", which is a fast way + to check if a remote repo is accessible and get a list of tags. The code here is more complex than _clone so we can print stdout and stderr interleaved in verbose mode while also capturing stdout separately. @@ -849,6 +852,11 @@ def _ls_remote( if verbose: print(line, end="") output.append(line.strip()) + if proc.returncode: + raise subprocess.CalledProcessError( + returncode=proc.returncode, + cmd=cmd, + ) return output From fd7a4d280bee66275df4c1471a28bfb4c8992e06 Mon Sep 17 00:00:00 2001 From: Zachary Lentz Date: Fri, 6 Sep 2024 16:54:20 -0700 Subject: [PATCH 6/6] STY: ruff format --- scripts/ioc_deploy.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/scripts/ioc_deploy.py b/scripts/ioc_deploy.py index 85e3c161..d629e0e6 100644 --- a/scripts/ioc_deploy.py +++ b/scripts/ioc_deploy.py @@ -221,7 +221,9 @@ def main_deploy(args: CliArgs) -> int: """ logger.info("Checking github connectivity") if not get_github_available(verbose=args.verbose): - logger.error("Github is not reachable, please check to make sure you're on a psbuild host.") + logger.error( + "Github is not reachable, please check to make sure you're on a psbuild host." + ) return ReturnCode.EXCEPTION logger.info("Checking repos and ioc deploy directories") @@ -353,7 +355,7 @@ def get_deploy_info(args: CliArgs) -> DeployInfo: if args.path_override: deploy_dir = args.path_override - if (args.name and args.release): + if args.name and args.release: name = args.name release = args.release else: @@ -399,9 +401,7 @@ def get_deploy_info(args: CliArgs) -> DeployInfo: ) if not args.path_override: - deploy_dir = get_target_dir( - name=name, ioc_dir=args.ioc_dir, release=release - ) + deploy_dir = get_target_dir(name=name, ioc_dir=args.ioc_dir, release=release) return DeployInfo(deploy_dir=deploy_dir, pkg_name=name, rel_name=release) @@ -464,7 +464,9 @@ def finalize_name(name: str, github_org: str, ioc_dir: str, verbose: bool) -> st area = find_casing_in_dir(dir=ioc_dir, name=area) except RuntimeError: logger.info("This is a new area, checking readme for casing") - name = casing_from_readme_clone(name=name, github_org=github_org, verbose=verbose) + name = casing_from_readme_clone( + name=name, github_org=github_org, verbose=verbose + ) logger.info(f"Using casing: {name}") return name logger.info(f"Using {area} as the area") @@ -473,7 +475,9 @@ def finalize_name(name: str, github_org: str, ioc_dir: str, verbose: bool) -> st suffix = find_casing_in_dir(dir=str(Path(ioc_dir) / area), name=suffix) except RuntimeError: logger.info("This is a new ioc, checking readme for casing") - casing = casing_from_readme_clone(name=name, github_org=github_org, verbose=verbose) + casing = casing_from_readme_clone( + name=name, github_org=github_org, verbose=verbose + ) # Use suffix from readme but keep area from directory search suffix = split_ioc_name(casing)[2] logger.info(f"Using {suffix} as the name") @@ -568,7 +572,9 @@ def finalize_tag(name: str, github_org: str, release: str, verbose: bool) -> str verbose=verbose, ) except subprocess.CalledProcessError as exc: - raise ValueError(f"Unable to access {github_org}/{name}, please make sure you have the correct access rights and the repository exists.") from exc + raise ValueError( + f"Unable to access {github_org}/{name}, please make sure you have the correct access rights and the repository exists." + ) from exc for rel in release_permutations(release=release): logger.debug(f"Trying variant {rel}") if rel in tags: @@ -838,7 +844,13 @@ def _ls_remote( Returns the stdout lines as a list of strings. """ - cmd = ["git", "ls-remote", "--tags", "--refs", f"git@github.com:{github_org}/{name}"] + cmd = [ + "git", + "ls-remote", + "--tags", + "--refs", + f"git@github.com:{github_org}/{name}", + ] kwds = { "stdout": subprocess.PIPE, "bufsize": 1,