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

Revert "fix(invoke): Write in UTF-8 string instead of bytes." #5401

Merged
merged 1 commit into from
Jun 22, 2023
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
4 changes: 2 additions & 2 deletions samcli/lib/utils/osutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def stdout():
io.BytesIO
Byte stream of Stdout
"""
return sys.stdout
return sys.stdout.buffer


def stderr():
Expand All @@ -99,7 +99,7 @@ def stderr():
io.BytesIO
Byte stream of stderr
"""
return sys.stderr
return sys.stderr.buffer


def remove(path):
Expand Down
7 changes: 2 additions & 5 deletions samcli/lib/utils/stream_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def __init__(self, stream, auto_flush=False):
def stream(self):
return self._stream

def write(self, output, encode=False, write_to_buffer=True):
def write(self, output, encode=False):
"""
Writes specified text to the underlying stream

Expand All @@ -31,10 +31,7 @@ def write(self, output, encode=False, write_to_buffer=True):
output bytes-like object
Bytes to write
"""
if write_to_buffer:
self._stream.buffer.write(output.encode() if encode else output)
else:
self._stream.write(output)
self._stream.write(output.encode() if encode else output)

if self._auto_flush:
self._stream.flush()
Expand Down
5 changes: 1 addition & 4 deletions samcli/local/docker/container.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""
Representation of a generic Docker container
"""
import json
import logging
import os
import pathlib
Expand Down Expand Up @@ -325,8 +324,7 @@ def wait_for_http_response(self, name, event, stdout):
data=event.encode("utf-8"),
timeout=(self.RAPID_CONNECTION_TIMEOUT, None),
)
stdout.write(json.dumps(json.loads(resp.content), ensure_ascii=False), write_to_buffer=False)
stdout.flush()
stdout.write(resp.content)

def wait_for_result(self, full_path, event, stdout, stderr, start_timer=None):
# NOTE(sriram-mv): Let logging happen in its own thread, so that a http request can be sent.
Expand Down Expand Up @@ -436,7 +434,6 @@ def _write_container_output(output_itr, stdout=None, stderr=None):

if stderr_data and stderr:
stderr.write(stderr_data)

except Exception as ex:
LOG.debug("Failed to get the logs from the container", exc_info=ex)

Expand Down
10 changes: 5 additions & 5 deletions samcli/local/docker/lambda_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def build(self, runtime, packagetype, image, layers, architecture, stream=None,
or not runtime
):
stream_writer = stream or StreamWriter(sys.stderr)
stream_writer.write("Building image...", write_to_buffer=False)
stream_writer.write("Building image...")
stream_writer.flush()
self._build_image(
image if image else base_image, rapid_image, downloaded_layers, architecture, stream=stream_writer
Expand Down Expand Up @@ -337,15 +337,15 @@ def set_item_permission(tar_info):
platform=get_docker_platform(architecture),
)
for log in resp_stream:
stream_writer.write(".", write_to_buffer=False)
stream_writer.write(".")
stream_writer.flush()
if "error" in log:
stream_writer.write("\n", write_to_buffer=False)
stream_writer.write("\n")
LOG.exception("Failed to build Docker Image")
raise ImageBuildException("Error building docker image: {}".format(log["error"]))
stream_writer.write("\n", write_to_buffer=False)
stream_writer.write("\n")
except (docker.errors.BuildError, docker.errors.APIError) as ex:
stream_writer.write("\n", write_to_buffer=False)
stream_writer.write("\n")
LOG.exception("Failed to build Docker Image")
raise ImageBuildException("Building Image failed.") from ex
finally:
Expand Down
8 changes: 3 additions & 5 deletions samcli/local/docker/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,16 @@ def pull_image(self, image_name, tag=None, stream=None):
raise DockerImagePullFailedException(str(ex)) from ex

# io streams, especially StringIO, work only with unicode strings
stream_writer.write(
"\nFetching {}:{} Docker container image...".format(image_name, tag), write_to_buffer=False
)
stream_writer.write("\nFetching {}:{} Docker container image...".format(image_name, tag))

# Each line contains information on progress of the pull. Each line is a JSON string
for _ in result_itr:
# For every line, print a dot to show progress
stream_writer.write(".", write_to_buffer=False)
stream_writer.write(".")
stream_writer.flush()

# We are done. Go to the next line
stream_writer.write("\n", write_to_buffer=False)
stream_writer.write("\n")

def has_image(self, image_name):
"""
Expand Down
21 changes: 0 additions & 21 deletions tests/integration/local/invoke/test_integrations_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,27 +291,6 @@ def test_invoke_returns_expected_result_when_no_event_given(self):
self.assertEqual(process.returncode, 0)
self.assertEqual("{}", process_stdout.decode("utf-8"))

# @pytest.mark.flaky(reruns=3)
def test_invoke_returns_utf8(self):
command_list = InvokeIntegBase.get_command_list(
"EchoEventFunction", template_path=self.template_path, event_path=self.event_utf8_path
)

process = Popen(command_list, stdout=PIPE)
try:
stdout, _ = process.communicate(timeout=TIMEOUT)
except TimeoutExpired:
process.kill()
raise

process_stdout = stdout.strip()

with open(self.event_utf8_path) as f:
expected_output = json.dumps(json.load(f), ensure_ascii=False)

self.assertEqual(process.returncode, 0)
self.assertEqual(expected_output, process_stdout.decode("utf-8"))

@pytest.mark.flaky(reruns=3)
def test_invoke_with_env_using_parameters(self):
command_list = InvokeIntegBase.get_command_list(
Expand Down
8 changes: 8 additions & 0 deletions tests/unit/lib/utils/test_osutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,29 @@ def test_raises_on_cleanup_failure(self, rmdir_mock):
@patch("os.rmdir")
def test_handles_ignore_error_case(self, rmdir_mock):
rmdir_mock.side_effect = OSError("fail")
dir_name = None
with osutils.mkdir_temp(ignore_errors=True) as tempdir:
dir_name = tempdir
self.assertTrue(os.path.exists(tempdir))


class Test_stderr(TestCase):
def test_must_return_sys_stderr(self):
expected_stderr = sys.stderr

if sys.version_info.major > 2:
expected_stderr = sys.stderr.buffer

self.assertEqual(expected_stderr, osutils.stderr())


class Test_stdout(TestCase):
def test_must_return_sys_stdout(self):
expected_stdout = sys.stdout

if sys.version_info.major > 2:
expected_stdout = sys.stdout.buffer

self.assertEqual(expected_stdout, osutils.stdout())


Expand Down
2 changes: 1 addition & 1 deletion tests/unit/lib/utils/test_stream_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def test_must_write_to_stream(self):
writer = StreamWriter(stream_mock)
writer.write(buffer)

stream_mock.buffer.write.assert_called_once_with(buffer)
stream_mock.write.assert_called_once_with(buffer)

def test_must_flush_underlying_stream(self):
stream_mock = Mock()
Expand Down
9 changes: 5 additions & 4 deletions tests/unit/local/docker/test_lambda_image.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import io
import tempfile

from unittest import TestCase
Expand Down Expand Up @@ -270,7 +271,7 @@ def test_force_building_image_that_doesnt_already_exists(
docker_client_mock.images.get.side_effect = ImageNotFound("image not found")
docker_client_mock.images.list.return_value = []

stream = Mock()
stream = io.StringIO()

lambda_image = LambdaImage(layer_downloader_mock, False, True, docker_client=docker_client_mock)
actual_image_id = lambda_image.build(
Expand Down Expand Up @@ -310,7 +311,7 @@ def test_force_building_image_on_daemon_404(
docker_client_mock.images.get.side_effect = NotFound("image not found")
docker_client_mock.images.list.return_value = []

stream = Mock()
stream = io.StringIO()

lambda_image = LambdaImage(layer_downloader_mock, False, True, docker_client=docker_client_mock)
actual_image_id = lambda_image.build(
Expand Down Expand Up @@ -350,7 +351,7 @@ def test_docker_distribution_api_error_on_daemon_api_error(
docker_client_mock.images.get.side_effect = APIError("error from docker daemon")
docker_client_mock.images.list.return_value = []

stream = Mock()
stream = io.StringIO()

lambda_image = LambdaImage(layer_downloader_mock, False, True, docker_client=docker_client_mock)
with self.assertRaises(DockerDistributionAPIError):
Expand All @@ -376,7 +377,7 @@ def test_not_force_building_image_that_doesnt_already_exists(
docker_client_mock.images.get.side_effect = ImageNotFound("image not found")
docker_client_mock.images.list.return_value = []

stream = Mock()
stream = io.StringIO()

lambda_image = LambdaImage(layer_downloader_mock, False, False, docker_client=docker_client_mock)
actual_image_id = lambda_image.build(
Expand Down
24 changes: 7 additions & 17 deletions tests/unit/local/docker/test_manager.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""
Tests container manager
"""

import io
import importlib
from unittest import TestCase
from unittest.mock import Mock, patch, MagicMock, ANY, call
Expand Down Expand Up @@ -216,29 +218,17 @@ def setUp(self):
self.manager = ContainerManager(docker_client=self.mock_docker_client)

def test_must_pull_and_print_progress_dots(self):
stream = Mock()
stream = io.StringIO()
pull_result = [1, 2, 3, 4, 5, 6, 7, 8, 9, 0]
self.mock_docker_client.api.pull.return_value = pull_result
expected_stream_calls = [
call(f"\nFetching {self.image_name}:latest Docker container image...", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call(".", write_to_buffer=False),
call("\n", write_to_buffer=False),
]
expected_stream_output = "\nFetching {}:latest Docker container image...{}\n".format(
self.image_name, "." * len(pull_result) # Progress bar will print one dot per response from pull API
)

self.manager.pull_image(self.image_name, stream=stream)

self.mock_docker_client.api.pull.assert_called_with(self.image_name, stream=True, decode=True, tag="latest")

stream.write.assert_has_calls(expected_stream_calls)
self.assertEqual(stream.getvalue(), expected_stream_output)

def test_must_raise_if_image_not_found(self):
msg = "some error"
Expand Down