make things cleaner

Author: nammn

scripts/release/branch_detection.py

@@ -21,10 +21,13 @@ def get_current_branch() -> Optional[str]:
             ["git", "rev-parse", "--abbrev-ref", "HEAD"], capture_output=True, text=True, check=True
         )
         branch = result.stdout.strip()
-        if branch and branch != "HEAD":
+        if branch == "HEAD":
+            return "master"
+        if branch != "":
             return branch
     except (subprocess.CalledProcessError, FileNotFoundError):
         return "master"
+    return "master"
 
 def get_cache_scope() -> str:
     """

scripts/release/build/image_build_process.py

@@ -185,7 +185,7 @@ def execute_docker_build(
             base_registry
         )
 
-        ensure_ecr_cache_repository(base_cache_repo)
+        # ensure_ecr_cache_repository(base_cache_repo)
 
         logger.info(f"Building image: {tag}")
         logger.info(f"Platforms: {platforms}")

scripts/release/tests/branch_detection_test.py

@@ -1,110 +1,76 @@
-import os
 import subprocess
 from unittest.mock import MagicMock, patch
 
-import pytest
-
 from scripts.release.branch_detection import (
     get_cache_scope,
     get_current_branch,
-    get_version_id,
-    is_evg_patch,
-    is_running_in_evg,
 )
 
 
 class TestGetCurrentBranch:
     """Test branch detection logic for different scenarios."""
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
     @patch("subprocess.run")
-    def test_local_development_master_branch(self, mock_run, mock_is_evg):
-        """Test local development on master branch."""
-        mock_is_evg.return_value = False
+    def test_master_branch(self, mock_run):
+        """Test detection of master branch."""
         mock_run.return_value = MagicMock(stdout="master\n", returncode=0)
 
         result = get_current_branch()
 
         assert result == "master"
         mock_run.assert_called_once_with(
-            ["git", "rev-parse", "--abbrev-ref", "HEAD"], capture_output=True, text=True, check=True
+            ["git", "rev-parse", "--abbrev-ref", "HEAD"],
+            capture_output=True,
+            text=True,
+            check=True
         )
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
     @patch("subprocess.run")
-    def test_local_development_feature_branch(self, mock_run, mock_is_evg):
-        """Test local development on feature branch."""
-        mock_is_evg.return_value = False
+    def test_feature_branch(self, mock_run):
+        """Test detection of feature branch."""
         mock_run.return_value = MagicMock(stdout="feature/new-cache\n", returncode=0)
 
         result = get_current_branch()
 
         assert result == "feature/new-cache"
+        mock_run.assert_called_once_with(
+            ["git", "rev-parse", "--abbrev-ref", "HEAD"],
+            capture_output=True,
+            text=True,
+            check=True
+        )
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
     @patch("subprocess.run")
-    def test_local_development_detached_head(self, mock_run, mock_is_evg):
-        """Test local development in detached HEAD state."""
-        mock_is_evg.return_value = False
+    def test_detached_head(self, mock_run):
+        """Test detection when in detached HEAD state."""
         mock_run.return_value = MagicMock(stdout="HEAD\n", returncode=0)
 
         result = get_current_branch()
 
         assert result == "master"  # fallback to master
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
     @patch("subprocess.run")
-    def test_local_development_git_error(self, mock_run, mock_is_evg):
-        """Test local development when git command fails."""
-        mock_is_evg.return_value = False
-        mock_run.side_effect = subprocess.CalledProcessError(1, "git")
+    def test_empty_output(self, mock_run):
+        """Test detection when git returns empty output."""
+        mock_run.return_value = MagicMock(stdout="\n", returncode=0)
 
         result = get_current_branch()
 
         assert result == "master"  # fallback to master
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("subprocess.run")
-    def test_evergreen_non_patch_build(self, mock_run, mock_is_patch, mock_is_evg):
-        """Test Evergreen non-patch build."""
-        mock_is_evg.return_value = True
-        mock_is_patch.return_value = False
-        mock_run.return_value = MagicMock(stdout="master\n", returncode=0)
-
-        result = get_current_branch()
-
-        assert result == "master"
-
-    @patch("scripts.release.branch_detection.is_running_in_evg")
-    @patch("scripts.release.branch_detection.is_evg_patch")
     @patch("subprocess.run")
-    def test_evergreen_patch_build_branch_detection(self, mock_run, mock_is_patch, mock_is_evg):
-        """Test Evergreen patch build with successful branch detection."""
-        mock_is_evg.return_value = True
-        mock_is_patch.return_value = True
-
-        # Mock git for-each-ref output
-        mock_run.side_effect = [
-            MagicMock(
-                stdout="origin/feature/cache-improvement abc123\norigin/evg-pr-test-123 abc123\norigin/main def456\n",
-                returncode=0,
-            ),
-            MagicMock(stdout="abc123\n", returncode=0),
-        ]
+    def test_git_command_fails(self, mock_run):
+        """Test fallback when git command fails."""
+        mock_run.side_effect = subprocess.CalledProcessError(1, "git")
 
         result = get_current_branch()
 
-        assert result == "feature/cache-improvement"
+        assert result == "master"  # fallback to master
 
-    @patch("scripts.release.branch_detection.is_running_in_evg")
-    @patch("scripts.release.branch_detection.is_evg_patch")
     @patch("subprocess.run")
-    def test_evergreen_patch_build_fallback(self, mock_run, mock_is_patch, mock_is_evg):
-        """Test Evergreen patch build fallback when branch detection fails."""
-        mock_is_evg.return_value = True
-        mock_is_patch.return_value = True
-        mock_run.side_effect = subprocess.CalledProcessError(1, "git")
+    def test_git_not_found(self, mock_run):
+        """Test fallback when git is not found."""
+        mock_run.side_effect = FileNotFoundError("git not found")
 
         result = get_current_branch()
 
@@ -114,124 +80,31 @@ def test_evergreen_patch_build_fallback(self, mock_run, mock_is_patch, mock_is_e
 class TestGetCacheScope:
     """Test cache scope generation for different scenarios."""
 
-    @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_master_branch_non_patch(self, mock_version_id, mock_is_patch, mock_branch):
-        """Test cache scope for master branch non-patch build."""
-        mock_branch.return_value = "master"
-        mock_is_patch.return_value = False
-        mock_version_id.return_value = None
-
-        result = get_cache_scope()
-
-        assert result == "master"
 
     @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_feature_branch_non_patch(self, mock_version_id, mock_is_patch, mock_branch):
-        """Test cache scope for feature branch non-patch build."""
+    def test_feature_branch(self, mock_branch):
+        """Test cache scope for feature branch."""
         mock_branch.return_value = "feature/new-cache"
-        mock_is_patch.return_value = False
-        mock_version_id.return_value = None
 
         result = get_cache_scope()
 
         assert result == "feature-new-cache"
 
     @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_patch_build_with_version_id(self, mock_version_id, mock_is_patch, mock_branch):
-        """Test cache scope for patch build with version ID."""
-        mock_branch.return_value = "feature/new-cache"
-        mock_is_patch.return_value = True
-        mock_version_id.return_value = "6899b7e35bfaee00077db986"
-
-        result = get_cache_scope()
-
-        assert result == "feature-new-cache-6899b7e3"
-
-    @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_patch_build_without_version_id(self, mock_version_id, mock_is_patch, mock_branch):
-        """Test cache scope for patch build without version ID."""
-        mock_branch.return_value = "feature/new-cache"
-        mock_is_patch.return_value = True
-        mock_version_id.return_value = None
-
-        result = get_cache_scope()
-
-        assert result == "feature-new-cache"
-
-    @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_branch_name_sanitization(self, mock_version_id, mock_is_patch, mock_branch):
+    def test_branch_name_sanitization(self, mock_branch):
         """Test branch name sanitization for cache scope."""
         mock_branch.return_value = "Feature/NEW_cache@123"
-        mock_is_patch.return_value = False
-        mock_version_id.return_value = None
 
         result = get_cache_scope()
 
         assert result == "feature-new_cache-123"
 
+
     @patch("scripts.release.branch_detection.get_current_branch")
-    @patch("scripts.release.branch_detection.is_evg_patch")
-    @patch("scripts.release.branch_detection.get_version_id")
-    def test_dependabot_branch(self, mock_version_id, mock_is_patch, mock_branch):
-        """Test cache scope for dependabot branch."""
-        mock_branch.return_value = "dependabot/npm_and_yarn/lodash-4.17.21"
-        mock_is_patch.return_value = False
-        mock_version_id.return_value = None
+    def test_complex_branch_name(self, mock_branch):
+        """Test cache scope for complex branch name with special characters."""
+        mock_branch.return_value = "user/feature-123_test.branch"
 
         result = get_cache_scope()
 
-        assert result == "dependabot-npm_and_yarn-lodash-4.17.21"
-
-
-class TestEnvironmentDetection:
-    """Test environment detection functions."""
-
-    def test_is_evg_patch_true(self):
-        """Test is_evg_patch returns True when is_patch is 'true'."""
-        with patch.dict(os.environ, {"is_patch": "true"}):
-            assert is_evg_patch() is True
-
-    def test_is_evg_patch_false(self):
-        """Test is_evg_patch returns False when is_patch is 'false'."""
-        with patch.dict(os.environ, {"is_patch": "false"}):
-            assert is_evg_patch() is False
-
-    def test_is_evg_patch_default(self):
-        """Test is_evg_patch returns False when is_patch is not set."""
-        with patch.dict(os.environ, {}, clear=True):
-            assert is_evg_patch() is False
-
-    def test_is_running_in_evg_true(self):
-        """Test is_running_in_evg returns True when RUNNING_IN_EVG is 'true'."""
-        with patch.dict(os.environ, {"RUNNING_IN_EVG": "true"}):
-            assert is_running_in_evg() is True
-
-    def test_is_running_in_evg_false(self):
-        """Test is_running_in_evg returns False when RUNNING_IN_EVG is 'false'."""
-        with patch.dict(os.environ, {"RUNNING_IN_EVG": "false"}):
-            assert is_running_in_evg() is False
-
-    def test_is_running_in_evg_default(self):
-        """Test is_running_in_evg returns False when RUNNING_IN_EVG is not set."""
-        with patch.dict(os.environ, {}, clear=True):
-            assert is_running_in_evg() is False
-
-    def test_get_version_id_set(self):
-        """Test get_version_id returns value when version_id is set."""
-        with patch.dict(os.environ, {"version_id": "6899b7e35bfaee00077db986"}):
-            assert get_version_id() == "6899b7e35bfaee00077db986"
-
-    def test_get_version_id_not_set(self):
-        """Test get_version_id returns None when version_id is not set."""
-        with patch.dict(os.environ, {}, clear=True):
-            assert get_version_id() is None
+        assert result == "user-feature-123_test.branch"

scripts/release/tests/image_build_process_test.py

@@ -2,71 +2,62 @@
 
 from scripts.release.build.image_build_process import (
     build_cache_configuration,
+    ensure_all_cache_repositories,
 )
 
 
 class TestBuildCacheConfiguration:
     """Test cache configuration building for different scenarios."""
 
     @patch("scripts.release.build.image_build_process.get_cache_scope")
-    @patch("scripts.release.build.image_build_process.get_current_branch")
-    def test_master_branch(self, mock_branch, mock_scope):
+    def test_master_branch(self, mock_scope):
         """Test cache configuration for master branch."""
-        mock_branch.return_value = "master"
         mock_scope.return_value = "master"
 
         base_registry = "268558157000.dkr.ecr.us-east-1.amazonaws.com/dev/cache/mongodb-kubernetes"
         cache_from, cache_to = build_cache_configuration(base_registry)
 
-        # Should read from master and shared
+        # Should read from master only
         expected_from = [
-            {"type": "registry", "ref": f"{base_registry}:master"},
-            {"type": "registry", "ref": f"{base_registry}:shared"},
+            {"type": "registry", "ref": f"{base_registry}:master"}
         ]
         assert cache_from == expected_from
 
         # Should write to master
-        assert len(cache_to) == 1
-        assert cache_to[0]["ref"] == f"{base_registry}:master"
-        assert cache_to[0]["mode"] == "max"
-        assert cache_to[0]["oci-mediatypes"] == "true"
-        assert cache_to[0]["image-manifest"] == "true"
+        assert cache_to["ref"] == f"{base_registry}:master"
+        assert cache_to["mode"] == "max"
+        assert cache_to["oci-mediatypes"] == "true"
+        assert cache_to["image-manifest"] == "true"
 
     @patch("scripts.release.build.image_build_process.get_cache_scope")
-    @patch("scripts.release.build.image_build_process.get_current_branch")
-    def test_feature_branch(self, mock_branch, mock_scope):
+    def test_feature_branch(self, mock_scope):
         """Test cache configuration for feature branch."""
-        mock_branch.return_value = "feature/new-cache"
         mock_scope.return_value = "feature-new-cache"
 
         base_registry = "268558157000.dkr.ecr.us-east-1.amazonaws.com/dev/cache/mongodb-kubernetes"
         cache_from, cache_to = build_cache_configuration(base_registry)
 
-        # Should read from branch, master, and shared
+        # Should read from branch and master
         expected_from = [
             {"type": "registry", "ref": f"{base_registry}:feature-new-cache"},
-            {"type": "registry", "ref": f"{base_registry}:master"},
-            {"type": "registry", "ref": f"{base_registry}:shared"},
+            {"type": "registry", "ref": f"{base_registry}:master"}
         ]
         assert cache_from == expected_from
 
-        # Should write to branch only (not master since we're not on master)
-        assert len(cache_to) == 1
-        assert cache_to[0]["ref"] == f"{base_registry}:feature-new-cache"
-        assert cache_to[0]["mode"] == "max"
-        assert cache_to[0]["oci-mediatypes"] == "true"
-        assert cache_to[0]["image-manifest"] == "true"
+        # Should write to branch only
+        assert cache_to["ref"] == f"{base_registry}:feature-new-cache"
+        assert cache_to["mode"] == "max"
+        assert cache_to["oci-mediatypes"] == "true"
+        assert cache_to["image-manifest"] == "true"
 
     @patch("scripts.release.build.image_build_process.get_cache_scope")
-    @patch("scripts.release.build.image_build_process.get_current_branch")
-    def test_patch_build_with_version_id(self, mock_branch, mock_scope):
-        """Test cache configuration for patch build with version ID."""
-        mock_branch.return_value = "feature/new-cache"
-        mock_scope.return_value = "feature-new-cache-6899b7e3"
+    def test_sanitized_branch_name(self, mock_scope):
+        """Test cache configuration with sanitized branch name."""
+        mock_scope.return_value = "feature-new-cache-123"
 
         base_registry = "268558157000.dkr.ecr.us-east-1.amazonaws.com/dev/cache/mongodb-kubernetes"
         cache_from, cache_to = build_cache_configuration(base_registry)
 
-        # Should include version ID in cache refs
-        assert cache_from[0]["ref"] == f"{base_registry}:feature-new-cache-6899b7e3"
-        assert cache_to[0]["ref"] == f"{base_registry}:feature-new-cache-6899b7e3"
+        # Should include sanitized branch name in cache refs
+        assert cache_from[0]["ref"] == f"{base_registry}:feature-new-cache-123"
+        assert cache_to["ref"] == f"{base_registry}:feature-new-cache-123"