Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logs on docker build failure #7675

Merged
merged 7 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ def _build_lambda_image(self, function_name: str, metadata: Dict, architecture:
LOG.debug("%s image is built for %s function", build_image, function_name)
except docker.errors.BuildError as ex:
LOG.error("Failed building function %s", function_name)
self._stream_lambda_image_build_logs(ex.build_log, function_name, False)
raise DockerBuildFailed(str(ex)) from ex

# The Docker-py low level api will stream logs back but if an exception is raised by the api
Expand All @@ -445,7 +446,9 @@ def _build_lambda_image(self, function_name: str, metadata: Dict, architecture:

return docker_tag

def _stream_lambda_image_build_logs(self, build_logs: List[Dict[str, str]], function_name: str) -> None:
def _stream_lambda_image_build_logs(
self, build_logs: List[Dict[str, str]], function_name: str, throw_on_error: bool = True
) -> None:
"""
Stream logs to the console from an Lambda image build.

Expand All @@ -456,7 +459,7 @@ def _stream_lambda_image_build_logs(self, build_logs: List[Dict[str, str]], func
function_name str
Name of the function that is being built
"""
build_log_streamer = LogStreamer(self._stream_writer)
build_log_streamer = LogStreamer(self._stream_writer, throw_on_error)
try:
build_log_streamer.stream_progress(build_logs)
except LogStreamError as ex:
Expand Down
10 changes: 8 additions & 2 deletions samcli/lib/docker/log_streamer.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ def __init__(self, msg: str) -> None:


class LogStreamer:
def __init__(self, stream: StreamWriter):
def __init__(self, stream: StreamWriter, throw_on_error: bool = True):
self._stream = stream
self._cursor_up_formatter = CursorUpFormatter()
self._cursor_down_formatter = CursorDownFormatter()
self._cursor_left_formatter = CursorLeftFormatter()
self._cursor_clear_formatter = ClearLineFormatter()
self._throw_on_error = throw_on_error

def stream_progress(self, logs: docker.APIClient.logs):
"""
Expand Down Expand Up @@ -73,7 +74,12 @@ def _stream_write(self, _id: str, status: str, stream: str, progress: str, error
:param error: docker log error
"""
if error:
raise LogStreamError(msg=error)
if self._throw_on_error:
raise LogStreamError(msg=error)
else:
self._stream.write_str(error)
return
Comment on lines -76 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we write this to the error stream, shouldn't we also throw it again? Changing this variable into opposite boolean to indicate whether an error should be written into stream or not could be better 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because you might have multiple error lines. Also the throw in this features case is with the whole Docker build exception as the content vs just an individual error line.


if not status and not stream:
return

Expand Down
19 changes: 16 additions & 3 deletions tests/unit/lib/build_module/test_app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -1728,13 +1728,26 @@ def test_can_build_image_function_under_debug_with_target(self, mock_os):
),
)

def test_can_raise_missing_dockerfile_error(self):
with self.assertRaises(DockerBuildFailed) as ex:
self.builder._build_lambda_image("Name", {}, X86_64)

self.assertEqual(ex.exception.args, ("Docker file or Docker context metadata are missed.",))

def test_can_raise_build_error(self):
self.docker_client_mock.images.build.side_effect = docker.errors.BuildError(
reason="Missing Dockerfile", build_log="Build failed"
reason="Build failure", build_log=[{"stream": "Some earlier log"}, {"error": "Build failed"}]
)

with self.assertRaises(DockerBuildFailed):
self.builder._build_lambda_image("Name", {}, X86_64)
with self.assertRaises(DockerBuildFailed) as ex:
self.builder._build_lambda_image("Name", {"Dockerfile": "Dockerfile", "DockerContext": "context"}, X86_64)

self.assertEqual(ex.exception.args, ("Build failure",))
self.assertEqual(self.stream_mock.write_str.call_count, 4, self.stream_mock.write_str.call_args_list)
self.assertEqual(
self.stream_mock.write_str.call_args_list,
[call("Some earlier log"), call(""), call("Build failed"), call(os.linesep)],
)


class TestApplicationBuilder_load_lambda_image_function(TestCase):
Expand Down
Loading