feat: modify run_docker_command to always return EntrypointOutput #613

devin-ai-integration · 2025-06-26T05:42:44Z

feat: modify run_docker_command to always return EntrypointOutput

Summary

This PR modifies the run_docker_command() function to always return an EntrypointOutput object instead of a raw subprocess.CompletedProcess, with the goal of improving error analysis and readability in standard test failures. The key changes include:

Always store STDOUT to disk for memory efficiency using temporary files
Return EntrypointOutput consistently to provide structured Airbyte protocol message parsing
Add backward compatibility properties (returncode, stderr) to EntrypointOutput
Update all standard test callers to use the new return type
Enhance error messages in test failures to show structured error information instead of raw stdout

The motivation for this change is that standard tests like test_docker_image_build_and_check currently produce unreadable failure outputs with raw stdout/stderr text, making debugging difficult. EntrypointOutput provides structured parsing of Airbyte protocol messages, cleaner error formatting, and access to specific message types.

Review & Testing Checklist for Human

Search codebase for any missed run_docker_command() callers - I updated the callers I found, but there may be others that expect CompletedProcess return type
Test actual Docker command failures to verify error messages are more readable than before and EntrypointOutput properties work correctly
Check temporary file handling - verify no disk space issues, proper cleanup, and that temporary files are readable by EntrypointOutput
Run standard tests with actual connectors to ensure no runtime errors from the type change
Verify CI passes completely - linting/typing passed locally, but full CI including integration tests is critical

Recommended test plan: Run test_docker_image_build_and_check with a connector that has known failures and compare error output quality before/after this change.

Diagram

graph TD
    A[airbyte_cdk/utils/docker.py]:::major-edit
    B[run_docker_command function]:::major-edit
    C[airbyte_cdk/test/entrypoint_wrapper.py]:::major-edit
    D[EntrypointOutput class]:::major-edit
    E[airbyte_cdk/test/standard_tests/docker_base.py]:::major-edit
    F[test_docker_image_build_and_spec]:::minor-edit
    G[test_docker_image_build_and_check]:::minor-edit
    H[test_docker_image_build_and_read]:::minor-edit
    I[verify_connector_image]:::minor-edit
    
    A --> B
    B --> |returns| D
    C --> D
    D --> |used by| E
    E --> F
    E --> G
    E --> H
    A --> I
    
    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit
        L3[Context/No Edit]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF

Notes

Memory efficiency: The change to always store stdout to disk prevents memory issues with large Docker command outputs
Type safety: All type hints have been updated and mypy passes with no issues
Backward compatibility: Added returncode and stderr properties to EntrypointOutput so existing code patterns still work
Error analysis improvement: Test failures now show structured error messages from Airbyte protocol instead of raw text dumps

Risk: This is a breaking change to a core utility function. While I've tried to maintain compatibility through EntrypointOutput properties, any code directly expecting CompletedProcess will need updates.

- Always store STDOUT to disk for memory efficiency - Add returncode and stderr properties to EntrypointOutput for backward compatibility - Update all callers in standard tests to use EntrypointOutput return type - Improve error analysis and readability in test failure outputs - Maintain strict backward compatibility through enhanced EntrypointOutput properties Co-Authored-By: AJ Steers <aj@airbyte.io>

devin-ai-integration · 2025-06-26T05:42:45Z

Original prompt from AJ Steers:

Received message in Slack channel #ask-devin-ai:

@Devin - In the CDK, specifically in the standard tests suite for `test_docker_image_build_and_check` and similar checks - the failures are not easy to analyze. We get a STDOUT with log messages, but they aren't readable. Can you adapt the `run_docker_command()` implementation to return an EntrypointOutput object? This class is adapted to better render failure outputs.

devin-ai-integration · 2025-06-26T05:42:46Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment and CI monitoring

Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers · 2025-06-26T05:46:17Z

airbyte_cdk/test/entrypoint_wrapper.py

@@ -68,6 +69,7 @@ def __init__(
        uncaught_exception: Optional[BaseException] = None,
        *,
        message_file: Path | None = None,
+        completed_process: Optional[subprocess.CompletedProcess[str]] = None,


Remove this from the constructor. Instead add a @classmethod factory function "from_completed_process()".

aaronsteers · 2025-06-26T05:46:57Z

airbyte_cdk/test/entrypoint_wrapper.py

+    @property
+    def returncode(self) -> int | None:
+        """Return the exit code of the process, if available."""
+        return self._completed_process.returncode if self._completed_process else None
+
+    @property
+    def stderr(self) -> str | None:
+        """Return the stderr output of the process, if available."""
+        return self._completed_process.stderr if self._completed_process else None
+


We can delete these as not needed.

@aaronsteers

- Remove completed_process parameter from constructor as requested - Add @classmethod from_completed_process() factory method - Remove returncode and stderr properties as they are not needed - Update callers to use EntrypointOutput.errors for failure detection - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

github-actions · 2025-06-26T05:56:09Z

PyTest Results (Fast)

3 669 tests ±0 3 658 ✅ ±0 6m 14s ⏱️ -8s
1 suites ±0 11 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit cd3419d. ± Comparison against base commit 0afea4a.

♻️ This comment has been updated with latest results.

aaronsteers · 2025-06-26T05:57:03Z

airbyte_cdk/test/entrypoint_wrapper.py

+        cls,
+        completed_process: subprocess.CompletedProcess[str],
+        *,
+        message_file: Path | None = None,


Let's call this stdout_file - since we're talking about the stdout stream in this context. (It's also the message file, but in the context of process completion, I think stdout_file is more clear.

@aaronsteers

- Rename parameter from message_file to stdout_file for better clarity - Update caller in run_docker_command() to use new parameter name - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers · 2025-06-26T06:02:15Z

airbyte_cdk/test/entrypoint_wrapper.py

-        """Create EntrypointOutput from a completed subprocess with optional message file."""
-        instance = cls(message_file=message_file)
+        """Create EntrypointOutput from a completed subprocess with optional stdout file."""
+        instance = cls(message_file=stdout_file)


If stdout_file is none, then completed_process.stdout might have valid output. Since stdout_file is optional, we should fall back to a "captured stdout" scenario, where stdout was captured in memory, and in that case would be available in the completed_process object.

@aaronsteers

- Handle case when stdout_file is None by falling back to completed_process.stdout - Create EntrypointOutput from captured stdout messages when no file provided - Maintain backward compatibility for both file-based and memory-based stdout - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers · 2025-06-26T06:03:59Z

airbyte_cdk/test/entrypoint_wrapper.py

@@ -76,6 +77,7 @@ def __init__(

        self._messages: list[AirbyteMessage] | None = None
        self._message_file: Path | None = message_file
+        self._completed_process: Optional[subprocess.CompletedProcess[str]] = None


I don't think we need _completed_process - if we can pass along all critical data during the from_completed_process() implementation, then we don't need to store the completed process object itself.

@aaronsteers

- Remove _completed_process field from EntrypointOutput class - No functionality lost as the attribute was never used after assignment - Simplifies the class by not storing unnecessary references - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers · 2025-06-26T06:06:12Z

airbyte_cdk/test/entrypoint_wrapper.py

+            messages = completed_process.stdout.splitlines()
+            instance = cls(messages=messages)
+        else:
+            instance = cls()


Let's raise a ValueError here. Either stdout_file should be provided, or completed_process.stdout should not be None.

@aaronsteers

…ted_process - Replace empty EntrypointOutput creation with ValueError - Add clear error message when both stdout_file and completed_process.stdout are None - Improves error handling and prevents silent failures - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers · 2025-06-26T06:10:52Z

airbyte_cdk/test/standard_tests/docker_base.py

+            if read_result.errors:
+                error_messages = [
+                    f"Error message: {error.trace.error.message}"
+                    for error in read_result.errors
+                    if error.trace and error.trace.error
+                ]
+                if not error_messages:
+                    error_messages = ["No structured error messages found"]


Devin - Let's move the "whet the heck happened?" logic into a method on the EntrypointOutput class.

Suggested change

if read_result.errors:

error_messages = [

f"Error message: {error.trace.error.message}"

for error in read_result.errors

if error.trace and error.trace.error

]

if not error_messages:

error_messages = ["No structured error messages found"]

if read_result.errors:

error_message = read_result.get_error_message()

@aaronsteers

- Move error message extraction logic from test code into reusable method - Simplify error handling in docker_base.py test - Improve code maintainability and reduce duplication - Addresses GitHub PR feedback from @aaronsteers Co-Authored-By: AJ Steers <aj@airbyte.io>

github-actions · 2025-06-26T06:30:51Z

PyTest Results (Full)

3 672 tests ±0 3 661 ✅ ±0 17m 49s ⏱️ -10s
1 suites ±0 11 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit cd3419d. ± Comparison against base commit 0afea4a.

github-actions bot added the enhancement New feature or request label Jun 26, 2025

fix: apply ruff formatting to resolve CI check failure

23ac759

Co-Authored-By: AJ Steers <aj@airbyte.io>

aaronsteers reviewed Jun 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: modify run_docker_command to always return EntrypointOutput #613

feat: modify run_docker_command to always return EntrypointOutput #613

Uh oh!

devin-ai-integration bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

devin-ai-integration bot commented Jun 26, 2025

Uh oh!

devin-ai-integration bot commented Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

github-actions bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

aaronsteers Jun 26, 2025

Uh oh!

github-actions bot commented Jun 26, 2025

Uh oh!

Uh oh!

feat: modify run_docker_command to always return EntrypointOutput #613

Are you sure you want to change the base?

feat: modify run_docker_command to always return EntrypointOutput #613

Uh oh!

Conversation

devin-ai-integration bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

feat: modify run_docker_command to always return EntrypointOutput

Summary

Review & Testing Checklist for Human

Diagram

Notes

Uh oh!

devin-ai-integration bot commented Jun 26, 2025

Uh oh!

devin-ai-integration bot commented Jun 26, 2025

🤖 Devin AI Engineer

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PyTest Results (Fast)

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

aaronsteers Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 26, 2025

PyTest Results (Full)

Uh oh!

Uh oh!

devin-ai-integration bot commented Jun 26, 2025 •

edited

Loading

github-actions bot commented Jun 26, 2025 •

edited

Loading