Change VCS info file format to JSON

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

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
-``<run-dir>/log/version/vcs.conf``.
+``<run-dir>/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
-``<run-dir>/log/version/uncommitted.diff``. (Note that git does not include
-untracked files in the diff.)
+``<run-dir>/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)

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

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 <http://www.gnu.org/licenses/>.
 
+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,22 +64,24 @@
 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(
         ['git', 'commit', '-am', '"Initial commit"'],
         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