Merge pull request #852 from MetaCell/bugfix/CH-268

CH-268: Fix name based exclusion in codefresh.py

Author: filippomc

tools/deployment-cli-tools/ch_cli_tools/codefresh.py

@@ -62,6 +62,24 @@ def clean_step_key(s: str) -> str:
     return re.sub(r'[^a-zA-Z0-9_]', '_', s)
 
 
+def dockerfile_selector_candidates(base_path: str, dockerfile_path: str) -> set[str]:
+    """Return stable identifiers that may be used with include/exclude selectors."""
+    relative_to_base = get_app_relative_to_base_path(base_path, dockerfile_path)
+    parent_name = relative_to_base.split("/")[0] if relative_to_base else ""
+    return {
+        candidate for candidate in (
+            relative_to_base,
+            app_name_from_path(relative_to_base),
+            parent_name,
+        ) if candidate
+    }
+
+
+def path_contains_excluded_segment(path: str, excluded_segments) -> bool:
+    normalized_path = f"/{path.replace(os.path.sep, '/')}/"
+    return any(f"/{segment}/" in normalized_path for segment in excluded_segments)
+
+
 def get_main_domain(url):
     try:
         url = url.split("//")[1].split("/")[0]
@@ -262,20 +280,23 @@ def codefresh_steps_from_base_path(base_path, fixed_context=None, include=build_
                 for dockerfile_path in find_dockerfiles_paths(base_path):
                     dockerfile_relative_to_root = relpath(dockerfile_path, '.')
                     dockerfile_relative_to_base = get_app_relative_to_base_path(base_path, dockerfile_path)
+                    selector_candidates = dockerfile_selector_candidates(base_path, dockerfile_path)
                     app_name = app_name_from_path(dockerfile_relative_to_base)
                     app_key = app_name
                     app_config: ApplicationHarnessConfig = app_key in helm_values.apps and helm_values.apps[app_key].harness
                     full_image_name = helm_values.apps[app_key].image if app_key in helm_values.apps\
                         else helm_values[KEY_TASK_IMAGES][app_key] if app_key in helm_values[KEY_TASK_IMAGES]\
                         else f"{base_name}/{app_name}"
 
-                    if include and not any(
-                            f"/{inc}/" in os.path.relpath(dockerfile_path, root_path) or dockerfile_path.endswith(f"/{inc}") for inc in include
-                    ):
+                    if include and not any(inc in selector_candidates for inc in include):
                         # Skip not included apps
                         continue
 
-                    if any(inc in dockerfile_path for inc in (list(exclude) + EXCLUDE_PATHS)):
+                    if any(ex in selector_candidates for ex in exclude):
+                        # Skip explicitly excluded apps/images
+                        continue
+
+                    if path_contains_excluded_segment(dockerfile_relative_to_root, EXCLUDE_PATHS):
                         # Skip excluded apps
                         continue
 

tools/deployment-cli-tools/ch_cli_tools/configurationgenerator.py

@@ -204,23 +204,64 @@ def _init_static_images(self, base_image_name):
                 )
 
     def _assign_static_build_dependencies(self, helm_values):
+        static_consumers = {}
         for static_img_dockerfile in self.static_images:
             key = os.path.basename(static_img_dockerfile)
             if key in helm_values[KEY_TASK_IMAGES]:
                 dependencies = guess_build_dependencies_from_dockerfile(
                     f"{static_img_dockerfile}")
                 for dep in dependencies:
+                    if dep in self.exclude and dep in helm_values[KEY_TASK_IMAGES] and key not in self.exclude:
+                        static_consumers.setdefault(dep, set()).add(key)
                     if dep in self.base_images and dep not in helm_values[KEY_TASK_IMAGES]:
                         helm_values[KEY_TASK_IMAGES][dep] = self.base_images[dep]
                         # helm_values.setdefault(KEY_TASK_IMAGES_BUILD, {})[dep] = {
                         #     'context': os.path.relpath(static_img_dockerfile, self.dest_deployment_path.parent),
                         #     'dockerfile': 'Dockerfile',
                         # }
 
-        for image_name in list(helm_values[KEY_TASK_IMAGES].keys()):
-            if image_name in self.exclude:
-                del helm_values[KEY_TASK_IMAGES][image_name]
-                # del helm_values[KEY_TASK_IMAGES_BUILD][image_name]
+        self._prune_excluded_task_images(helm_values, extra_consumers=static_consumers)
+
+    def _get_excluded_task_image_consumers(self, values):
+        """Return reverse dependencies for excluded task images from app build deps."""
+        task_images = values.get(KEY_TASK_IMAGES, {})
+        apps = values.get(KEY_APPS, {})
+        consumers = {}
+
+        for app_name, app_values in apps.items():
+            if app_name in self.exclude:
+                continue
+            build_dependencies = app_values.get(KEY_HARNESS, {}).get('dependencies', {}).get('build', [])
+            if not build_dependencies:
+                continue
+            for dep in build_dependencies:
+                if dep in self.exclude and dep in task_images:
+                    consumers.setdefault(dep, set()).add(app_name)
+
+        return consumers
+
+    def _prune_excluded_task_images(self, values, extra_consumers=None):
+        """Exclude task images only when no non-excluded image still depends on them."""
+        task_images = values.get(KEY_TASK_IMAGES, {})
+        if not task_images:
+            return
+
+        consumers = self._get_excluded_task_image_consumers(values)
+        if extra_consumers:
+            for image_name, image_consumers in extra_consumers.items():
+                consumers.setdefault(image_name, set()).update(image_consumers)
+
+        for image_name in list(task_images.keys()):
+            if image_name not in self.exclude:
+                continue
+            used_by = sorted(consumers.get(image_name, set()))
+            if used_by:
+                logging.warning(
+                    "Image %s was excluded but is still required by non-excluded builds: %s. Keeping it.",
+                    image_name, ", ".join(used_by)
+                )
+                continue
+            del task_images[image_name]
 
     def _init_base_images(self, base_image_name):
         """Initialize base images (infrastructure/base-images/) with root context."""
@@ -600,6 +641,7 @@ def validate_dependencies(values):
     all_apps = {a for a in values["apps"]}
     for app in all_apps:
         app_values = values["apps"][app]
+        app_task_images = set(app_values.get(KEY_TASK_IMAGES, {}))
         if 'dependencies' in app_values[KEY_HARNESS]:
             soft_dependencies = {
                 d for d in app_values[KEY_HARNESS]['dependencies']['soft']}
@@ -617,9 +659,9 @@ def validate_dependencies(values):
             build_dependencies = {
                 d for d in app_values[KEY_HARNESS]['dependencies']['build']}
 
+            available_builds = set(values.get(KEY_TASK_IMAGES, {})) | all_apps | app_task_images
             not_found = {
-                d for d in build_dependencies if d not in values[KEY_TASK_IMAGES]}
-            not_found = {d for d in not_found if d not in all_apps}
+                d for d in build_dependencies if d not in available_builds}
             if not_found:
                 raise ValuesValidationException(
                     f"Bad build dependencies specified for application {app}: {','.join(not_found)} not found as built image")
@@ -629,8 +671,10 @@ def validate_dependencies(values):
 
             not_found = {d for d in service_dependencies if d not in all_apps}
             if not_found:
-                raise ValuesValidationException(
-                    f"Bad service application dependencies specified for application {app}: {','.join(not_found)}")
+                logging.warning(
+                    f"Service proxy (use_services) dependencies for application {app} are not deployed: "
+                    f"{','.join(not_found)}. Proxies to these services will not be created."
+                )
 
 
 def collect_apps_helm_templates(search_root, dest_helm_chart_path, templates_path=HELM_PATH, exclude=(), include=None, envs=()):

tools/deployment-cli-tools/ch_cli_tools/helm.py

@@ -170,6 +170,8 @@ def _aggregate_task_images(self, values):
                 if image:
                     values[KEY_TASK_IMAGES][dep_name] = image
                 app_name = dep_name
+            elif dep_name in self.base_images:
+                values[KEY_TASK_IMAGES][dep_name] = self.base_images[dep_name]
             elif prefix in apps:
                 app_name = prefix
                 values[KEY_TASK_IMAGES][dep_name] = apps[app_name][KEY_TASK_IMAGES][dep_name]
@@ -178,10 +180,7 @@ def _aggregate_task_images(self, values):
                     if key in included_builds or app_name in self.include:
                         values[KEY_TASK_IMAGES][key] = apps[app_name][KEY_TASK_IMAGES][key]
 
-        for ex in self.exclude:
-            if ex in values[KEY_TASK_IMAGES]:
-                logging.info(f"Excluding {ex} from build")
-                del values[KEY_TASK_IMAGES][ex]
+        self._prune_excluded_task_images(values)
 
     def __finish_helm_values(self, values, defer_task_images=False):
         """
@@ -279,6 +278,8 @@ def __finish_helm_values(self, values, defer_task_images=False):
                         if image:
                             values[KEY_TASK_IMAGES][dep_name] = image
                         app_name = dep_name
+                    elif dep_name in self.base_images:
+                        values[KEY_TASK_IMAGES][dep_name] = self.base_images[dep_name]
                     elif prefix in apps:  # build dependency within an application that is not part of the deployment
                         app_name = prefix
                         values[KEY_TASK_IMAGES][dep_name] = apps[app_name][KEY_TASK_IMAGES][dep_name]
@@ -294,10 +295,7 @@ def __finish_helm_values(self, values, defer_task_images=False):
                 values[KEY_TASK_IMAGES].update(apps[v][KEY_TASK_IMAGES])
 
         if not defer_task_images:
-            for ex in self.exclude:
-                if ex in values[KEY_TASK_IMAGES]:
-                    logging.info(f"Excluding {ex} from build")
-                    del values[KEY_TASK_IMAGES][ex]
+            self._prune_excluded_task_images(values)
         create_env_variables(values)
         return values, self.include
 

tools/deployment-cli-tools/tests/test_codefresh.py

@@ -381,6 +381,50 @@ def test_create_codefresh_configuration_nobuild():
     assert "publish_myapp-mytask" in l1_steps["publish"]["steps"]
 
 
+def test_excluding_common_app_does_not_skip_common_images_dependencies():
+    values = create_helm_chart(
+        [CLOUDHARNESS_ROOT, RESOURCES],
+        output_path=OUT,
+        include=['myapp'],
+        exclude=['common'],
+        domain='my.local',
+        namespace='test',
+        env='dev',
+        local=False,
+        tag=1,
+        registry='reg'
+    )
+    try:
+        values[KEY_TASK_IMAGES]['my-common'] = 'reg/testprojectname/my-common:1'
+
+        root_paths = preprocess_build_overrides(
+            root_paths=[CLOUDHARNESS_ROOT, RESOURCES],
+            helm_values=values,
+            merge_build_path=BUILD_MERGE_DIR
+        )
+
+        build_included = [app['harness']['name']
+                          for app in values['apps'].values() if 'harness' in app]
+
+        cf = create_codefresh_deployment_scripts(root_paths, include=build_included,
+                                                 exclude=['common'],
+                                                 envs=['dev'],
+                                                 base_image_name=values['name'],
+                                                 helm_values=values, save=False)
+
+        all_build_steps = {
+            step_name: step
+            for build_step_name in [STEP_0, STEP_1, STEP_2, STEP_3]
+            if build_step_name in cf['steps']
+            for step_name, step in cf['steps'][build_step_name]['steps'].items()
+        }
+
+        assert 'my-common' in all_build_steps, \
+            'Excluding app "common" must not skip the my-common image under infrastructure/common-images'
+    finally:
+        shutil.rmtree(BUILD_MERGE_DIR, ignore_errors=True)
+
+
 def test_codefresh_db_connect_string_secret():
     """When an app has database.connect_string set to '', a custom_values entry must be added to the deployment step."""
     values = create_helm_chart(

tools/deployment-cli-tools/tests/test_helm.py

@@ -197,9 +197,35 @@ def test_collect_helm_values_wrong_dependencies_validate(tmp_path):
     with pytest.raises(ValuesValidationException):
         create_helm_chart([CLOUDHARNESS_ROOT, f"{RESOURCES}/wrong-dependencies"], output_path=out_folder, domain="my.local",
                           namespace='test', env='prod', local=False, tag=1, include=["wrong-build"])
-    with pytest.raises(ValuesValidationException):
+    try:
         create_helm_chart([CLOUDHARNESS_ROOT, f"{RESOURCES}/wrong-dependencies"], output_path=out_folder, domain="my.local",
                           namespace='test', env='prod', local=False, tag=1, include=["wrong-services"])
+    except ValuesValidationException:
+        pytest.fail("Should not error because of missing use_services dependency")
+
+
+def test_validate_dependencies_accepts_app_local_build_images():
+    values = {
+        KEY_APPS: {
+            'portal': {
+                KEY_HARNESS: {
+                    'dependencies': {
+                        'soft': [],
+                        'hard': [],
+                        'build': ['cloudharness-base', 'cloudharness-django'],
+                    },
+                    'use_services': [],
+                },
+                KEY_TASK_IMAGES: {
+                    'cloudharness-base': 'reg/project/cloudharness-base:1',
+                    'cloudharness-django': 'reg/project/cloudharness-django:1',
+                },
+            }
+        },
+        KEY_TASK_IMAGES: {},
+    }
+
+    validate_dependencies(values)
 
 
 def test_collect_helm_values_build_dependencies(tmp_path):
@@ -441,13 +467,12 @@ def test_exclude_single_task(tmp_path):
 
     assert "myapp-mytask" not in values["task-images"], "myapp-mytask has been excluded, so should not appear in the task images"
 
-    try:
-        values = create_helm_chart([CLOUDHARNESS_ROOT, RESOURCES], output_path=out_folder, domain="my.local",
-                                   env='fulldep', local=False, include=["dependantapp"], exclude=["myapp-mytask"])
+    values = create_helm_chart([CLOUDHARNESS_ROOT, RESOURCES], output_path=out_folder, domain="my.local",
+                               env='fulldep', local=False, include=["dependantapp"], exclude=["myapp-mytask"])
 
-        assert False, "myapp-mytask has been excluded, but also declared as a dependency, so should not be excluded"
-    except ValuesValidationException as e:
-        pass
+    assert "myapp-mytask" in values[KEY_TASK_IMAGES], (
+        "myapp-mytask is excluded but still required by dependantapp, so it should be kept"
+    )
 
 
 def test_app_depends_on_app(tmp_path):