From d7254058b99180bf6cba99f38d039e434f387639 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 7 Dec 2021 17:44:21 +0000 Subject: [PATCH 1/2] Change VCS info file format to JSON --- cylc/flow/install_plugins/log_vc_info.py | 90 ++++++++++--------- .../functional/post-install/00-log-vc-info.t | 4 +- tests/unit/post_install/test_log_vc_info.py | 47 ++++++++-- 3 files changed, 88 insertions(+), 53 deletions(-) diff --git a/cylc/flow/install_plugins/log_vc_info.py b/cylc/flow/install_plugins/log_vc_info.py index a317fafbe7d..05cf7851994 100644 --- a/cylc/flow/install_plugins/log_vc_info.py +++ b/cylc/flow/install_plugins/log_vc_info.py @@ -19,43 +19,50 @@ If the workflow source directory is a supported repository/working copy (git or svn), information about the working copy will be saved in -``/log/version/vcs.conf``. +``/log/version/vcs.json``. An example of this information for a git repo: -.. code-block:: cylc +.. code-block:: json - version control system = "git" - repository version = "2.8.0-dirty" - commit = "e5dc6573dd70cabd8f973d1535c17c29c026d553" - working copy root path = "~/cylc-src/my-workflow-git" - status = \"\"\" - M flow.cylc - \"\"\" + { + "version control system": "git", + "repository version": "2.8.0-dirty", + "commit": "e5dc6573dd70cabd8f973d1535c17c29c026d553", + "working copy root path": "~/cylc-src/my-workflow-git", + "status": [ + " M flow.cylc", + "M bin/thing.sh" + ] + } And for an svn working copy: -.. code-block:: cylc - - version control system = "svn" - working copy root path = "~/cylc-src/my-workflow-svn" - url = "file:///home/my-workflow-svn/trunk" - repository uuid = "219f5687-8eb8-44b1-beb6-e8220fa964d3" - revision = "14" - status = \"\"\" - M flow.cylc - \"\"\" +.. code-block:: json + { + "version control system": "svn", + "working copy root path": "~/cylc-src/my-workflow-svn", + "url": "file:///home/my-workflow-svn/trunk", + "repository uuid": "219f5687-8eb8-44b1-beb6-e8220fa964d3", + "revision": "14", + "status": [ + "M readme.txt" + ] + } Any uncommitted changes will also be saved as a diff in -``/log/version/uncommitted.diff``. (Note that git does not include -untracked files in the diff.) +``/log/version/uncommitted.diff``. + +.. note:: + + Git does not include untracked files in the diff. """ -from collections import OrderedDict +import json from pathlib import Path from subprocess import Popen, DEVNULL, PIPE -from typing import Dict, Iterable, List, Optional, TYPE_CHECKING, Union +from typing import Any, Dict, Iterable, List, Optional, TYPE_CHECKING, Union from cylc.flow import LOG from cylc.flow.exceptions import CylcError @@ -107,6 +114,9 @@ LOG_VERSION_DIR = Path(WorkflowFiles.LOG_DIR, 'version') +DIFF_FILENAME = 'uncommitted.diff' +INFO_FILENAME = 'vcs.json' +JSON_INDENT = 4 class VCSNotInstalledError(CylcError): @@ -139,10 +149,10 @@ def __str__(self) -> str: return f"{self.vcs} repository at {self.path} is missing a base commit" -def get_vc_info(path: Union[Path, str]) -> Optional['OrderedDict[str, str]']: +def get_vc_info(path: Union[Path, str]) -> Optional[Dict[str, Any]]: """Return the version control information for a repository, given its path. """ - info = OrderedDict() + info: Dict[str, Any] = {} missing_base = False for vcs, args in INFO_COMMANDS.items(): try: @@ -200,7 +210,7 @@ def _run_cmd(vcs: str, args: Iterable[str], cwd: Union[Path, str]) -> str: stderr=PIPE, text=True, ) - # commands are defined in constants at top of module + # (nosec because commands are defined in constants at top of module) except FileNotFoundError as exc: # This will only be raised if the VCS command is not installed, # otherwise Popen() will succeed with a non-zero return code @@ -217,7 +227,7 @@ def _run_cmd(vcs: str, args: Iterable[str], cwd: Union[Path, str]) -> str: def write_vc_info( - info: 'OrderedDict[str, str]', run_dir: Union[Path, str] + info: Dict[str, Any], run_dir: Union[Path, str] ) -> None: """Write version control info to the workflow's vcs log dir. @@ -227,16 +237,12 @@ def write_vc_info( """ if not info: raise ValueError("Nothing to write") - info_file = Path(run_dir, LOG_VERSION_DIR, 'vcs.conf') - info_file.parent.mkdir(exist_ok=True) + info_file = Path(run_dir, LOG_VERSION_DIR, INFO_FILENAME) + info_file.parent.mkdir(exist_ok=True, parents=True) with open(info_file, 'w') as f: - for key, value in info.items(): - if key == 'status': - f.write(f"{key} = \"\"\"\n") - f.write(f"{value}\n") - f.write("\"\"\"\n") - else: - f.write(f"{key} = \"{value}\"\n") + f.write( + json.dumps(info, indent=JSON_INDENT) + ) def _get_git_commit(path: Union[Path, str]) -> str: @@ -245,20 +251,20 @@ def _get_git_commit(path: Union[Path, str]) -> str: return _run_cmd(GIT, args, cwd=path).splitlines()[0] -def get_status(vcs: str, path: Union[Path, str]) -> str: - """Return the short status of a repo. +def get_status(vcs: str, path: Union[Path, str]) -> List[str]: + """Return the short status of a repo, as a list of lines. Args: vcs: The version control system. path: The path to the repository. """ args = STATUS_COMMANDS[vcs] - return _run_cmd(vcs, args, cwd=path).rstrip('\n') + return _run_cmd(vcs, args, cwd=path).rstrip('\n').split('\n') -def _parse_svn_info(info_text: str) -> 'OrderedDict[str, str]': +def _parse_svn_info(info_text: str) -> Dict[str, Any]: """Return OrderedDict of certain info parsed from svn info raw output.""" - ret = OrderedDict() + ret: Dict[str, Any] = {} for line in info_text.splitlines(): if line: key, value = (ln.strip() for ln in line.split(':', 1)) @@ -299,7 +305,7 @@ def write_diff(diff: str, run_dir: Union[Path, str]) -> None: diff: The diff. run_dir: The workflow run directory. """ - diff_file = Path(run_dir, LOG_VERSION_DIR, 'uncommitted.diff') + diff_file = Path(run_dir, LOG_VERSION_DIR, DIFF_FILENAME) diff_file.parent.mkdir(exist_ok=True) with open(diff_file, 'w') as f: f.write(diff) diff --git a/tests/functional/post-install/00-log-vc-info.t b/tests/functional/post-install/00-log-vc-info.t index c8d8e1a89df..07697a248a9 100644 --- a/tests/functional/post-install/00-log-vc-info.t +++ b/tests/functional/post-install/00-log-vc-info.t @@ -37,10 +37,10 @@ git commit -am 'Initial commit' run_ok "${TEST_NAME_BASE}-install" cylc install -VCS_INFO_FILE="${RND_WORKFLOW_RUNDIR}/runN/log/version/vcs.conf" +VCS_INFO_FILE="${RND_WORKFLOW_RUNDIR}/runN/log/version/vcs.json" exists_ok "$VCS_INFO_FILE" # Basic check, unit tests cover this in more detail: -contains_ok "$VCS_INFO_FILE" <<< 'version control system = "git"' +grep_ok '"version control system": "git"' "$VCS_INFO_FILE" -F DIFF_FILE="${RND_WORKFLOW_RUNDIR}/runN/log/version/uncommitted.diff" exists_ok "$DIFF_FILE" # Expected to be empty but should exist diff --git a/tests/unit/post_install/test_log_vc_info.py b/tests/unit/post_install/test_log_vc_info.py index debb4001638..cddb8012997 100644 --- a/tests/unit/post_install/test_log_vc_info.py +++ b/tests/unit/post_install/test_log_vc_info.py @@ -14,16 +14,23 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . +import json from pathlib import Path import pytest from pytest import MonkeyPatch, TempPathFactory import shutil import subprocess -from typing import Any, Tuple +from typing import Any, Callable, Tuple from unittest.mock import Mock from cylc.flow.install_plugins.log_vc_info import ( - get_diff, _get_git_commit, get_status, get_vc_info, main + INFO_FILENAME, + LOG_VERSION_DIR, + get_diff, + _get_git_commit, + get_status, + get_vc_info, + main, ) Fixture = Any @@ -57,15 +64,15 @@ def git_source_repo(tmp_path_factory: TempPathFactory) -> Tuple[str, str]: """Init a git repo for a workflow source dir. - The repo has a flow.cylc file with uncommitted changes. This dir is reused + The repo has uncommitted changes. This dir is reused by all tests requesting it in this module. Returns (source_dir_path, commit_hash) """ - source_dir: Path = tmp_path_factory.getbasetemp().joinpath('git_repo') + source_dir: Path = tmp_path_factory.getbasetemp() / 'git_repo' source_dir.mkdir() subprocess.run(['git', 'init'], cwd=source_dir, check=True) - flow_file = source_dir.joinpath('flow.cylc') + flow_file = source_dir / 'flow.cylc' flow_file.write_text(BASIC_FLOW_1) subprocess.run(['git', 'add', '-A'], cwd=source_dir, check=True) subprocess.run( @@ -73,6 +80,8 @@ def git_source_repo(tmp_path_factory: TempPathFactory) -> Tuple[str, str]: cwd=source_dir, check=True, capture_output=True) # Overwrite file to introduce uncommitted changes: flow_file.write_text(BASIC_FLOW_2) + # Also add new file: + (source_dir / 'gandalf.md').touch() commit_sha = subprocess.run( ['git', 'rev-parse', 'HEAD'], cwd=source_dir, check=True, capture_output=True, text=True @@ -126,7 +135,10 @@ def test_get_git_commit(git_source_repo: Tuple[str, str]): def test_get_status_git(git_source_repo: Tuple[str, str]): """Test get_status() for a git repo""" source_dir, commit_sha = git_source_repo - assert get_status('git', source_dir) == " M flow.cylc" + assert get_status('git', source_dir) == [ + " M flow.cylc", + "?? gandalf.md" + ] @require_git @@ -140,7 +152,10 @@ def test_get_vc_info_git(git_source_repo: Tuple[str, str]): ('repository version', f"{commit_sha[:7]}-dirty"), ('commit', commit_sha), ('working copy root path', source_dir), - ('status', " M flow.cylc") + ('status', [ + " M flow.cylc", + "?? gandalf.md" + ]) ] assert list(vc_info.items()) == expected @@ -158,6 +173,20 @@ def test_get_diff_git(git_source_repo: Tuple[str, str]): assert line in diff_lines +@require_git +def test_main_git(git_source_repo: Tuple[str, str], tmp_run_dir: Callable): + """Test the written JSON info file.""" + source_dir, _ = git_source_repo + run_dir: Path = tmp_run_dir('frodo') + main(source_dir, None, run_dir) + with open(run_dir / LOG_VERSION_DIR / INFO_FILENAME, 'r') as f: + loaded = json.loads(f.read()) + assert isinstance(loaded, dict) + assert loaded['version control system'] == 'git' + assert isinstance(loaded['status'], list) + assert len(loaded['status']) == 2 + + @require_svn def test_get_vc_info_svn(svn_source_repo: Tuple[str, str, str]): """Test get_vc_info() for an svn working copy""" @@ -170,7 +199,7 @@ def test_get_vc_info_svn(svn_source_repo: Tuple[str, str, str]): ('url', f"file://{repo_path}/project/trunk"), ('repository uuid', uuid), ('revision', "1"), - ('status', "M flow.cylc") + ('status', ["M flow.cylc"]) ] assert list(vc_info.items()) == expected @@ -223,7 +252,7 @@ def test_no_base_commit_git(tmp_path: Path): expected = [ ('version control system', "git"), ('working copy root path', str(source_dir)), - ('status', "?? flow.cylc") + ('status', ["?? flow.cylc"]) ] assert list(vc_info.items()) == expected assert get_diff('git', source_dir) is None From 0851a112eb42e1e107c0252c2fa43171083f284d Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 7 Dec 2021 18:37:17 +0000 Subject: [PATCH 2/2] Changelog [skip ci] --- CHANGES.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 5fdedf54713..2c1801b913f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -73,8 +73,11 @@ renamed `cylc get-resources` and small changes made: - Metadata as well as names from ``--list`` option. - Files extracted to to ``target/source_name`` rather than ``target/full/source/path``. +[#4548](https://github.com/cylc/cylc-flow/pull/4548) - Changed the +workflow version control info log file format from modified-INI to JSON. + [#4521](https://github.com/cylc/cylc-flow/pull/4521) - The workflow config -logs (that get written in `log/flow-config/` on start/restart/reload) +logs (that get written in `log/flow-config/` on start/restart/reload) are now sparse, i.e. they will no longer be fleshed-out with defaults. ### Fixes