[BUG] ImageSpec NoOpBuilder should always return True while calling should_build function (flyteorg#3311)

Signed-off-by: Alex Wu <[email protected]>
Signed-off-by: Atharva <[email protected]>

Author: popojk

flytekit/image_spec/noop_builder.py

@@ -6,6 +6,20 @@ class NoOpBuilder(ImageSpecBuilder):
 
     builder_type = "noop"
 
+    def should_build(self, image_spec: ImageSpec) -> bool:
+        """
+        The build_image function of NoOpBuilder does not actually build a Docker image.
+        Since no Docker build process occurs, we do not need to check for Docker daemon
+        or existing images. Therefore, should_build should always return True.
+
+        Args:
+            image_spec (ImageSpec): Image specification
+
+        Returns:
+            bool: Always returns True
+        """
+        return True
+
     def build_image(self, image_spec: ImageSpec) -> str:
         if not isinstance(image_spec.base_image, str):
             msg = "base_image must be a string to use the noop image builder"

tests/flytekit/unit/core/image_spec/test_image_spec.py

@@ -359,3 +359,38 @@ def test_with_builder_options():
         "existing_builder_option_1": "existing_builder_option_value_1",
         "new_builder_option_1": "new_builder_option_value_1"
     }
+
+def test_noop_builder_updates_image_name_mapping():
+    from flytekit.image_spec.noop_builder import NoOpBuilder
+
+    # Clear any existing mappings to ensure clean test state
+    ImageBuildEngine._REGISTRY.pop("noop", None)
+    ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear()
+
+    # Register NoOpBuilder
+    ImageBuildEngine.register("noop", NoOpBuilder())
+
+    # Create an image spec with NoOpBuilder
+    expected_image_name = "localhost:30000/test_image:latest"
+    image_spec = ImageSpec(
+        name="test_image",
+        builder="noop",
+        base_image=expected_image_name
+    )
+
+    # Get the image name before building
+    img_name = image_spec.image_name()
+
+    # Build the image
+    ImageBuildEngine.build(image_spec)
+
+    # Verify that the mapping was created in _IMAGE_NAME_TO_REAL_NAME
+    assert img_name in ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME
+
+    # Verify the mapping value is correct
+    actual_real_name = ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME[img_name]
+    assert actual_real_name == expected_image_name
+
+    # Clean up
+    ImageBuildEngine._REGISTRY.pop("noop", None)
+    ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear()

tests/flytekit/unit/core/image_spec/test_noop_builder.py

@@ -22,3 +22,19 @@ def test_noop_builder_error(base_image):
     image_spec = ImageSpec(base_image=base_image)
     with pytest.raises(ValueError, match=msg):
         builder.build_image(image_spec)
+
+
+def test_noop_builder_should_build():
+    """Test that NoOpBuilder.should_build always returns True."""
+    builder = NoOpBuilder()
+
+    # Test with different image specs to ensure should_build always returns True
+    test_cases = [
+        ImageSpec(base_image="localhost:30000/flytekit"),
+        ImageSpec(base_image="python:3.9"),
+        ImageSpec(base_image="custom/image:latest"),
+    ]
+
+    for image_spec in test_cases:
+        result = builder.should_build(image_spec)
+        assert result is True, f"should_build should return True for {image_spec.base_image}"