bind samcli version to certain commit of app template (#3815)

bind samcli version to certain commit of app template

samcli/commands/init/init_templates.py

@@ -12,7 +12,13 @@
 
 from samcli.cli.global_config import GlobalConfig
 from samcli.commands.exceptions import UserException, AppTemplateUpdateException
-from samcli.lib.utils.git_repo import GitRepo, CloneRepoException, CloneRepoUnstableStateException
+from samcli.lib.utils import configuration
+from samcli.lib.utils.git_repo import (
+    GitRepo,
+    CloneRepoException,
+    CloneRepoUnstableStateException,
+    ManifestNotFoundException,
+)
 from samcli.lib.utils.packagetype import IMAGE
 from samcli.local.common.runtime_template import (
     RUNTIME_DEP_TEMPLATE_MAPPING,
@@ -23,7 +29,10 @@
 )
 
 LOG = logging.getLogger(__name__)
-MANIFEST_URL = "https://raw.githubusercontent.com/aws/aws-sam-cli-app-templates/master/manifest-v2.json"
+APP_TEMPLATES_REPO_COMMIT = configuration.get_app_template_repo_commit()
+MANIFEST_URL = (
+    f"https://raw.githubusercontent.com/aws/aws-sam-cli-app-templates/{APP_TEMPLATES_REPO_COMMIT}/manifest-v2.json"
+)
 APP_TEMPLATES_REPO_URL = "https://github.com/aws/aws-sam-cli-app-templates"
 APP_TEMPLATES_REPO_NAME = "aws-sam-cli-app-templates"
 
@@ -66,7 +75,12 @@ def clone_templates_repo(self):
         if not self._git_repo.clone_attempted:
             shared_dir: Path = GlobalConfig().config_dir
             try:
-                self._git_repo.clone(clone_dir=shared_dir, clone_name=APP_TEMPLATES_REPO_NAME, replace_existing=True)
+                self._git_repo.clone(
+                    clone_dir=shared_dir,
+                    clone_name=APP_TEMPLATES_REPO_NAME,
+                    replace_existing=True,
+                    commit=APP_TEMPLATES_REPO_COMMIT,
+                )
             except CloneRepoUnstableStateException as ex:
                 raise AppTemplateUpdateException(str(ex)) from ex
             except (OSError, CloneRepoException):
@@ -215,7 +229,16 @@ def _get_manifest(self):
         try:
             response = requests.get(MANIFEST_URL, timeout=10)
             body = response.text
-        except (requests.Timeout, requests.ConnectionError):
+            # if the commit is not exist then MANIFEST_URL will be invalid, fall back to use manifest in latest commit
+            if response.status_code == 404:
+                LOG.warning(
+                    "Request to MANIFEST_URL: %s failed, the commit hash in this url maybe invalid, "
+                    "Using manifest.json in the latest commit instead.",
+                    MANIFEST_URL,
+                )
+                raise ManifestNotFoundException()
+
+        except (requests.Timeout, requests.ConnectionError, ManifestNotFoundException):
             LOG.debug("Request to get Manifest failed, attempting to clone the repository")
             self.clone_templates_repo()
             manifest_path = self.get_manifest_path()

samcli/lib/utils/configuration.py

@@ -0,0 +1,27 @@
+""" Read info from runtime_config.json file"""
+
+import json
+from pathlib import Path
+import logging
+
+from click import ClickException
+
+LOG = logging.getLogger(__name__)
+
+CONFIG_FILE = Path(Path(__file__).resolve().parents[2], "runtime_config.json")
+config = json.loads(CONFIG_FILE.read_text())
+
+
+def get_app_template_repo_commit():
+    """
+    Returns
+    -------
+    the value of app_template_repo_commit
+
+    """
+    commit_hash = config.get("app_template_repo_commit", None)
+    if not commit_hash:
+        raise ClickException(
+            message="Error when retrieving app_template_repo_commit, runtime_config.json file maybe invalid"
+        )
+    return commit_hash

samcli/lib/utils/git_repo.py

@@ -30,6 +30,12 @@ class CloneRepoUnstableStateException(CloneRepoException):
     """
 
 
+class ManifestNotFoundException(Exception):
+    """
+    Exception class when request Manifest file return 404.
+    """
+
+
 class GitRepo:
     """
     Class for managing a Git repo, currently it has a clone functionality only
@@ -81,7 +87,7 @@ def _git_executable() -> str:
 
         raise OSError("Cannot find git, was looking at executables: {}".format(executables))
 
-    def clone(self, clone_dir: Path, clone_name: str, replace_existing: bool = False) -> Path:
+    def clone(self, clone_dir: Path, clone_name: str, replace_existing: bool = False, commit: str = "") -> Path:
         """
         creates a local clone of this Git repository.
         This method is different from the standard Git clone in the following:
@@ -98,7 +104,8 @@ def clone(self, clone_dir: Path, clone_name: str, replace_existing: bool = False
             The dirname of the local clone
         replace_existing: bool
             Whether to replace the current local clone directory if already exists or not
-
+        commit: str
+            if a commit is provided, it will checkout out the commit in the clone repo
         Returns
         -------
             The path of the created local clone
@@ -127,6 +134,12 @@ def clone(self, clone_dir: Path, clone_name: str, replace_existing: bool = False
                     cwd=tempdir,
                     stderr=subprocess.STDOUT,
                 )
+
+                # bind a certain sam cli release to a specific commit of the aws-sam-cli-app-templates's repo, avoiding
+                # regression
+                if commit:
+                    self._checkout_commit(temp_path, commit)
+
                 self.local_path = self._persist_local_repo(temp_path, clone_dir, clone_name, replace_existing)
                 return self.local_path
             except OSError as ex:
@@ -162,3 +175,18 @@ def _persist_local_repo(temp_path: str, dest_dir: Path, dest_name: str, replace_
                 f"Check that you have permissions to create/delete files in {dest_dir} directory "
                 "or file an issue at https://github.com/aws/aws-sam-cli/issues"
             ) from ex
+
+    @staticmethod
+    def _checkout_commit(repo_dir: str, commit: str):
+        try:
+            # if the checkout commit failed, it will use the latest commit instead
+            git_executable = GitRepo._git_executable()
+            check_output(
+                [git_executable, "checkout", commit],
+                cwd=repo_dir,
+                stderr=subprocess.STDOUT,
+            )
+        except subprocess.CalledProcessError as checkout_error:
+            output = checkout_error.output.decode("utf-8")
+            if "fatal" in output.lower() or "error" in output.lower():
+                LOG.warning("WARN: Commit not exist: %s, using the latest one", commit, exc_info=checkout_error)

samcli/runtime_config.json

@@ -0,0 +1,3 @@
+{
+  "app_template_repo_commit" : "09b7de41c32ee5f50ec8ceeec4a304534767844b"
+}

tests/integration/init/test_init_command.py

@@ -12,6 +12,8 @@
 
 TIMEOUT = 300
 
+COMMIT_ERROR = "WARN: Commit not exist:"
+
 
 class TestBasicInitCommand(TestCase):
     def test_init_command_passes_and_dir_created(self):
@@ -33,16 +35,20 @@ def test_init_command_passes_and_dir_created(self):
                     "--no-interactive",
                     "-o",
                     temp,
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_passes_and_dir_created_image(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -89,16 +95,20 @@ def test_init_new_app_template(self):
                     "--no-interactive",
                     "-o",
                     temp,
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "qs-scratch").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_java_maven(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -117,16 +127,20 @@ def test_init_command_java_maven(self):
                     "--no-interactive",
                     "-o",
                     temp,
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app-maven").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_java_gradle(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -145,16 +159,20 @@ def test_init_command_java_gradle(self):
                     "--no-interactive",
                     "-o",
                     temp,
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app-gradle").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_with_extra_context_parameter(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -175,16 +193,20 @@ def test_init_command_with_extra_context_parameter(self):
                     '{"schema_name": "codedeploy", "schema_type": "aws"}',
                     "-o",
                     temp,
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app-maven").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_passes_with_arm_architecture(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -205,16 +227,20 @@ def test_init_command_passes_with_arm_architecture(self):
                     temp,
                     "--architecture",
                     "arm64",
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_passes_with_x86_64_architecture(self):
         with tempfile.TemporaryDirectory() as temp:
@@ -235,16 +261,20 @@ def test_init_command_passes_with_x86_64_architecture(self):
                     temp,
                     "--architecture",
                     "x86_64",
-                ]
+                ],
+                stdout=PIPE,
+                stderr=PIPE,
             )
             try:
-                process.communicate(timeout=TIMEOUT)
+                stdout_data, stderr_data = process.communicate(timeout=TIMEOUT)
+                stderr = stderr_data.decode("utf-8")
             except TimeoutExpired:
                 process.kill()
                 raise
 
             self.assertEqual(process.returncode, 0)
             self.assertTrue(Path(temp, "sam-app").is_dir())
+            self.assertNotIn(COMMIT_ERROR, stderr)
 
     def test_init_command_passes_with_unknown_architecture(self):
         with tempfile.TemporaryDirectory() as temp:

tests/unit/lib/utils/test_configuration.py

@@ -0,0 +1,7 @@
+from unittest.case import TestCase
+from samcli.lib.utils import configuration
+
+
+class TestConfiguration(TestCase):
+    def test_return_correct_value(self):
+        self.assertEqual(configuration.get_app_template_repo_commit(), "09b7de41c32ee5f50ec8ceeec4a304534767844b")

tests/unit/lib/utils/test_git_repo.py

@@ -9,6 +9,7 @@
 REPO_NAME = "REPO NAME"
 CLONE_DIR = os.path.normpath("/tmp/local/clone/dir")
 EXPECTED_DEFAULT_CLONE_PATH = os.path.normpath(os.path.join(CLONE_DIR, REPO_NAME))
+COMMIT = "123"
 
 
 class TestGitRepo(TestCase):
@@ -186,3 +187,37 @@ def test_clone_when_failed_to_move_cloned_repo_from_temp_to_final_destination(
         shutil_mock.rmtree.assert_called_once_with(EXPECTED_DEFAULT_CLONE_PATH, onerror=rmtree_callback)
         shutil_mock.copytree.assert_called_once_with(ANY, EXPECTED_DEFAULT_CLONE_PATH, ignore=ANY)
         shutil_mock.ignore_patterns.assert_called_once_with("*.git")
+
+    @patch("samcli.lib.utils.git_repo.LOG")
+    @patch("samcli.lib.utils.git_repo.check_output")
+    def test_checkout_commit_when_commit_not_exist(self, check_output_mock, log_mock):
+        check_output_mock.side_effect = subprocess.CalledProcessError(
+            "fail", "fail", "fatal: reference is not a tree".encode("utf-8")
+        )
+        try:
+            with self.assertRaises(CloneRepoException):
+                self.repo._checkout_commit(repo_dir="test", commit="1234")
+        except Exception:
+            pass
+        log_mock.warning.assert_called()
+
+    @patch("samcli.lib.utils.git_repo.Path.exists")
+    @patch("samcli.lib.utils.git_repo.shutil")
+    @patch("samcli.lib.utils.git_repo.check_output")
+    @patch("samcli.lib.utils.git_repo.subprocess.Popen")
+    @patch("samcli.lib.utils.git_repo.platform.system")
+    def test_clone_with_commit(self, platform_mock, popen_mock, check_output_mock, shutil_mock, path_exist_mock):
+        path_exist_mock.return_value = False
+        self.repo.clone(clone_dir=self.local_clone_dir, clone_name=REPO_NAME, commit=COMMIT)
+        self.local_clone_dir.mkdir.assert_called_once_with(mode=0o700, parents=True, exist_ok=True)
+        popen_mock.assert_has_calls(
+            [call(["git"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)],
+            [call(["git"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)],
+        )
+        check_output_mock.assert_has_calls(
+            [call(["git", "clone", self.repo.url, REPO_NAME], cwd=ANY, stderr=subprocess.STDOUT)],
+            [call(["git", "checkout", COMMIT], cwd=ANY, stderr=subprocess.STDOUT)],
+        )
+        shutil_mock.rmtree.assert_not_called()
+        shutil_mock.copytree.assert_called_with(ANY, EXPECTED_DEFAULT_CLONE_PATH, ignore=ANY)
+        shutil_mock.ignore_patterns.assert_called_with("*.git")