Merge pull request #287 from jakub-nt/CFE-4089

CFE-4089: Implemented updating modules added from a URL by branch

Author: olehermanse

JSON.md

@@ -194,6 +194,10 @@ The modules inside `build`, `provides`, and `index` use these fields:
 - `commit` (string): Commit hash used when we download and snapshot the version of a module.
   Used in `index` and modules added from an index.
   Must be updated together with `version`.
+- `branch` (string): Branch name used when updating modules added by URL.
+  Optional - if specified, the branch will be used during `cfbs update` instead of the default branch.
+  Requires the `url` field to be specified.
+  If the `branch` field is specified, the `commit` field must also be specified.
 - `subdirectory` (string): Used if the module is inside a subdirectory of a repo.
   See for example [the `cfbs.json` of our modules repo](https://github.com/cfengine/modules/blob/master/cfbs.json).
   Not used for local modules (policy files or folders) - the name is the path to the module in this case.

cfbs/cfbs_config.py

@@ -150,27 +150,32 @@ def _add_using_url(
         checksum=None,
         explicit_build_steps=None,
     ):
-        """The `url` argument can optionally also have the form `<url>@<commit>`."""
+        """The `url` argument can optionally also have the form `<url>@<commit or branch>`."""
         url_commit = None
+        url_branch = None
         if url.endswith(SUPPORTED_ARCHIVES):
             config_path, url_commit = fetch_archive(url, checksum)
         else:
             assert url.startswith(SUPPORTED_URI_SCHEMES)
 
-            commit = None
+            reference = None
             if "@" in url and (url.rindex("@") > url.rindex(".")):
-                # commit specified in the url
-                url, commit = url.rsplit("@", 1)
+                # commit or branch specified together with the URL
+                url, reference = url.rsplit("@", 1)
                 if "@" in url and (url.rindex("@") > url.rindex(".")):
                     raise CFBSUserError(
-                        "Cannot specify more than one commit for one add URL"
+                        "Cannot specify more than one commit or branch for one add URL"
                     )
-                if not is_a_commit_hash(commit):
-                    raise CFBSExitError("'%s' is not a commit reference" % commit)
+            config_path, url_commit = clone_url_repo(url, reference)
 
-            config_path, url_commit = clone_url_repo(url, commit)
+            # a branch name and a commit hash are distinguished as follows:
+            # if the reference matches the form of a commit hash, it is assumed to be a commit hash, otherwise it is assumed to be a branch name
+            if not is_a_commit_hash(reference):
+                url_branch = reference
 
-        remote_config = CFBSJson(path=config_path, url=url, url_commit=url_commit)
+        remote_config = CFBSJson(
+            path=config_path, url=url, url_commit=url_commit, url_branch=url_branch
+        )
 
         provides = remote_config.get_provides(added_by)
         add_all = True

cfbs/cfbs_json.py

@@ -20,7 +20,7 @@
 from cfbs.utils import CFBSValidationError, read_json, CFBSExitError
 
 
-def _construct_provided_module(name, data, url, commit, added_by):
+def _construct_provided_module(name, data, url, commit, branch, added_by):
     # At this point the @commit part should be removed from url so:
     # either url should not have an @,
     # or the @ should be for user@host.something
@@ -35,6 +35,8 @@ def _construct_provided_module(name, data, url, commit, added_by):
     module["description"] = data["description"]
     module["url"] = url
     module["commit"] = commit
+    if branch is not None:
+        module["branch"] = branch
     subdirectory = data.get("subdirectory")
     if subdirectory:
         module["subdirectory"] = subdirectory
@@ -60,12 +62,16 @@ def __init__(
         index_argument=None,
         data=None,
         url=None,
-        url_commit=None,
+        url_commit: Optional[str] = None,
+        url_branch=None,
     ):
         assert path
         self.path = path
+
         self.url = url
         self.url_commit = url_commit
+        self.url_branch = url_branch
+
         if data:
             self._data = data
         else:
@@ -184,7 +190,7 @@ def get_provides(self, added_by: Optional[str]):
             )
         for k, v in self._data["provides"].items():
             module = _construct_provided_module(
-                k, v, self.url, self.url_commit, added_by
+                k, v, self.url, self.url_commit, self.url_branch, added_by
             )
             modules[k] = module
         return modules
@@ -194,7 +200,7 @@ def get_module_for_build(self, name, added_by="cfbs add"):
         if "provides" in self._data and name in self._data["provides"]:
             module = self._data["provides"][name]
             return _construct_provided_module(
-                name, module, self.url, self.url_commit, added_by
+                name, module, self.url, self.url_commit, self.url_branch, added_by
             )
         if name in self.index:
             return self.index.get_module_object(name, added_by)

cfbs/commands.py

@@ -113,7 +113,6 @@ def search_command(terms: List[str]):
     git_configure_and_initialize,
     is_git_repo,
     CFBSGitError,
-    ls_remote,
 )
 
 from cfbs.git_magic import commit_after_command, git_commit_maybe_prompt
@@ -281,16 +280,12 @@ def init_command(
         log.debug("--masterfiles=%s appears to be a version number" % masterfiles)
         to_add = ["masterfiles@%s" % masterfiles]
     elif masterfiles != "no":
-        log.debug("--masterfiles=%s appears to be a branch" % masterfiles)
-        branch = masterfiles
-        remote = "https://github.com/cfengine/masterfiles"
-        commit = ls_remote(remote, branch)
-        if commit is None:
-            raise CFBSExitError(
-                "Failed to find branch or tag %s at remote %s" % (branch, remote)
-            )
-        log.debug("Current commit for masterfiles branch %s is %s" % (branch, commit))
-        to_add = ["%s@%s" % (remote, commit), "masterfiles"]
+        log.debug(
+            "--masterfiles=%s appears to be a branch or a commit hash" % masterfiles
+        )
+        # add masterfiles from URL, instead of index by name
+        MPF_REPO_URL = "https://github.com/cfengine/masterfiles"
+        to_add = ["%s@%s" % (MPF_REPO_URL, masterfiles), "masterfiles"]
     if to_add:
         result = add_command(to_add, added_by="cfbs init")
         if result != 0:
@@ -623,9 +618,10 @@ def update_command(to_update):
             log.warning("Module '%s' not in build. Skipping its update." % update.name)
             continue
         if "url" in old_module:
-            path, commit = clone_url_repo(old_module["url"])
+            branch = old_module.get("branch")
+            path, commit = clone_url_repo(old_module["url"], branch)
             remote_config = CFBSJson(
-                path=path, url=old_module["url"], url_commit=commit
+                path=path, url=old_module["url"], url_commit=commit, url_branch=branch
             )
 
             module_name = old_module["name"]
@@ -716,7 +712,7 @@ def update_command(to_update):
             )
         raise CFBSValidationError(
             "The cfbs.json was invalid before update, "
-            + "but updating modules did not fix it - aborting update"
+            + "but updating modules did not fix it - aborting update "
             + "(see validation error messages above)"
         )
     config.save()
@@ -958,6 +954,7 @@ def human_readable(key: str):
         "url",
         "repo",
         "version",
+        "branch",
         "commit",
         "by",
         "status",

cfbs/git.py

@@ -249,3 +249,16 @@ def git_check_tracked_changes(scope=["all"]):
         raise CFBSGitError(
             "Failed to run 'git status -s -u' to check for changes."
         ) from cpe
+
+
+def treeish_exists(treeish, repo_path):
+    command = [
+        "git",
+        "rev-parse",
+        "--verify",
+        "--end-of-options",
+        treeish + r"^{object}",
+    ]
+    result = run(command, cwd=repo_path, stdout=DEVNULL, stderr=DEVNULL, check=False)
+
+    return result.returncode == 0

cfbs/internal_file_management.py

@@ -13,6 +13,7 @@
 import shutil
 from typing import Optional
 
+from cfbs.git import ls_remote, treeish_exists
 from cfbs.utils import (
     cfbs_dir,
     cp,
@@ -136,28 +137,19 @@ def _get_path_from_url(url):
     return path
 
 
-def _get_git_repo_commit_sha(repo_path):
-    assert os.path.isdir(os.path.join(repo_path, ".git"))
-
-    with open(os.path.join(repo_path, ".git", "HEAD"), "r") as f:
-        head_ref_info = f.read()
-
-    assert head_ref_info.startswith("ref: ")
-    head_ref = head_ref_info[5:].strip()
-
-    with open(os.path.join(repo_path, ".git", head_ref)) as f:
-        return f.read().strip()
-
-
 def _clone_and_checkout(url, path, treeish):
     # NOTE: If any of these shell (git) commands fail, we will exit
     if not os.path.exists(os.path.join(path, ".git")):
         sh("git clone --no-checkout %s %s" % (url, path))
+    if not treeish_exists(treeish, path):
+        raise CFBSExitError("%s not found in %s" % (treeish, url))
+
     sh("git checkout " + treeish, directory=path)
 
 
-def clone_url_repo(repo_url: str, commit: Optional[str] = None):
-    """Clones a Git repository at `repo_url` URL, optionally checking out the `commit` commit.
+def clone_url_repo(repo_url: str, reference: Optional[str] = None):
+    """Clones a Git repository at `repo_url` URL, optionally checking out the `reference` commit or branch.
+    If `reference` is `None`, the repository's default branch will be used for the checkout.
 
     Returns path to the `cfbs.json` located in the cloned Git repository, and the Git commit hash.
     """
@@ -170,20 +162,23 @@ def clone_url_repo(repo_url: str, commit: Optional[str] = None):
     repo_dir = os.path.join(downloads, repo_path)
     os.makedirs(repo_dir, exist_ok=True)
 
-    if commit is not None:
-        commit_path = os.path.join(repo_dir, commit)
-        _clone_and_checkout(repo_url, commit_path, commit)
+    if reference is None:
+        reference = "HEAD"
+
+    # always store versions of the repository in cfbs/downloads by commit hash
+    # therefore for branches, first find the commit it points to
+    if is_a_commit_hash(reference):
+        commit = reference
     else:
-        master_path = os.path.join(repo_dir, "master")
-        sh("git clone %s %s" % (repo_url, master_path))
-        commit = _get_git_repo_commit_sha(master_path)
-
-        commit_path = os.path.join(repo_dir, commit)
-        if os.path.exists(commit_path):
-            # Already cloned in the commit dir, just remove the 'master' clone
-            sh("rm -rf %s" % master_path)
-        else:
-            sh("mv %s %s" % (master_path, commit_path))
+        # `reference` is a branch
+        commit = ls_remote(repo_url, reference)
+        if commit is None:
+            raise CFBSExitError(
+                "Failed to find branch %s at %s" % (reference, repo_url)
+            )
+
+    commit_path = os.path.join(repo_dir, commit)
+    _clone_and_checkout(repo_url, commit_path, commit)
 
     json_path = os.path.join(commit_path, "cfbs.json")
     if os.path.exists(json_path):

cfbs/module.py

@@ -70,6 +70,7 @@ def attributes() -> tuple:
         return (
             "name",
             "version",
+            "branch",
             "commit",
             "added_by",
             "steps",

cfbs/pretty.py

@@ -21,6 +21,7 @@
     "index",
     "version",
     "commit",
+    "branch",
     "subdirectory",
     "dependencies",
     "added_by",

cfbs/utils.py

@@ -12,7 +12,7 @@
 import urllib.error
 from collections import OrderedDict
 from shutil import rmtree
-from typing import Iterable, List, Tuple, Union
+from typing import Iterable, List, Optional, Tuple, Union
 
 from cfbs.pretty import pretty
 
@@ -532,7 +532,9 @@ def fetch_url(url, target, checksum=None):
         ) from e
 
 
-def is_a_commit_hash(commit):
+def is_a_commit_hash(commit: Optional[str]):
+    if commit is None:
+        return False
     return bool(SHA1_RE.match(commit) or SHA256_RE.match(commit))
 
 

cfbs/validate.py

@@ -502,6 +502,17 @@ def _validate_module_commit(name, module):
         raise CFBSValidationError(name, '"commit" must be a commit reference')
 
 
+def _validate_module_branch(name, module):
+    assert "branch" in module
+    branch = module["branch"]
+    if type(branch) is not str:
+        raise CFBSValidationError(name, '"branch" must be of type string')
+    if not module["url"]:
+        raise CFBSValidationError(name, '"branch" key requires the "url" key')
+    if not module["commit"]:
+        raise CFBSValidationError(name, '"branch" key requires the "commit" key')
+
+
 def _validate_module_subdirectory(name, module):
     assert "subdirectory" in module
     if type(module["subdirectory"]) is not str:
@@ -741,6 +752,8 @@ def validate_single_module(context, name, module, config, local_check=False):
         _validate_module_version(name, module)
     if "commit" in module:
         _validate_module_commit(name, module)
+    if "branch" in module:
+        _validate_module_branch(name, module)
     if "subdirectory" in module:
         _validate_module_subdirectory(name, module)
     if "steps" in module: