openstack/python-openstackclient
Code Issues Proposed changes

image: Simplify handling of data provided via stdin

Browse Source 
This was unnecessarily complex.

Change-Id: I8289d5ce7356d8bc89425590a7f71bca91a6d396
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
This commit is contained in:
Stephen Finucane 2022-10-21 16:40:55 +01:00
parent bafece762a
commit 3d9a9df935
2 changed files with 82 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

91
openstackclient/image/v2/image.py
View File

@ -134,34 +134,32 @@ def _get_member_columns(item):
) )
def get_data_file(args): def get_data_from_stdin():
if args.file: # distinguish cases where:
return (open(args.file, 'rb'), args.file) # (1) stdin is not valid (as in cron jobs):
else: # openstack ... <&-
# distinguish cases where: # (2) image data is provided through stdin:
# (1) stdin is not valid (as in cron jobs): # openstack ... < /tmp/file
# openstack ... <&- # (3) no image data provided
# (2) image data is provided through stdin: # openstack ...
# openstack ... < /tmp/file try:
# (3) no image data provided os.fstat(0)
# openstack ... except OSError:
try: # (1) stdin is not valid
os.fstat(0) return None
except OSError:
# (1) stdin is not valid
return (None, None)
if not sys.stdin.isatty():
# (2) image data is provided through stdin
image = sys.stdin
if hasattr(sys.stdin, 'buffer'):
image = sys.stdin.buffer
if msvcrt:
msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)
return (image, None) if not sys.stdin.isatty():
else: # (2) image data is provided through stdin
# (3) image = sys.stdin
return (None, None) if hasattr(sys.stdin, 'buffer'):
image = sys.stdin.buffer
if msvcrt:
msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)
return image
else:
# (3)
return None
class AddProjectToImage(command.ShowOne): class AddProjectToImage(command.ShowOne):
@ -277,6 +275,7 @@ class CreateImage(command.ShowOne):
source_group = parser.add_mutually_exclusive_group() source_group = parser.add_mutually_exclusive_group()
source_group.add_argument( source_group.add_argument(
"--file", "--file",
dest="filename",
metavar="<file>", metavar="<file>",
help=_("Upload image from local file"), help=_("Upload image from local file"),
) )
@ -299,7 +298,10 @@ class CreateImage(command.ShowOne):
"--progress", "--progress",
action="store_true", action="store_true",
default=False, default=False,
help=_("Show upload progress bar."), help=_(
"Show upload progress bar "
"(ignored if passing data via stdin)"
),
) )
parser.add_argument( parser.add_argument(
'--sign-key-path', '--sign-key-path',
@ -463,7 +465,15 @@ class CreateImage(command.ShowOne):
# open the file first to ensure any failures are handled before the # open the file first to ensure any failures are handled before the
# image is created. Get the file name (if it is file, and not stdin) # image is created. Get the file name (if it is file, and not stdin)
# for easier further handling. # for easier further handling.
fp, fname = get_data_file(parsed_args) if parsed_args.filename:
try:
fp = open(parsed_args.filename, 'rb')
except FileNotFoundError:
raise exceptions.CommandError(
'%r is not a valid file' % parsed_args.filename,
)
else:
fp = get_data_from_stdin()
if fp is not None and parsed_args.volume: if fp is not None and parsed_args.volume:
msg = _( msg = _(
@ -472,26 +482,24 @@ class CreateImage(command.ShowOne):
) )
raise exceptions.CommandError(msg) raise exceptions.CommandError(msg)
if fp is None and parsed_args.file: if parsed_args.progress and parsed_args.filename:
LOG.warning(_("Failed to get an image file."))
return {}, {}
if parsed_args.progress and parsed_args.file:
# NOTE(stephenfin): we only show a progress bar if the user # NOTE(stephenfin): we only show a progress bar if the user
# requested it *and* we're reading from a file (not stdin) # requested it *and* we're reading from a file (not stdin)
filesize = os.path.getsize(fname) filesize = os.path.getsize(parsed_args.filename)
if filesize is not None: if filesize is not None:
kwargs['validate_checksum'] = False kwargs['validate_checksum'] = False
kwargs['data'] = progressbar.VerboseFileWrapper(fp, filesize) kwargs['data'] = progressbar.VerboseFileWrapper(fp, filesize)
elif fname: else:
kwargs['filename'] = fname kwargs['data'] = fp
elif parsed_args.filename:
kwargs['filename'] = parsed_args.filename
elif fp: elif fp:
kwargs['validate_checksum'] = False kwargs['validate_checksum'] = False
kwargs['data'] = fp kwargs['data'] = fp
# sign an image using a given local private key file # sign an image using a given local private key file
if parsed_args.sign_key_path or parsed_args.sign_cert_id: if parsed_args.sign_key_path or parsed_args.sign_cert_id:
if not parsed_args.file: if not parsed_args.filename:
msg = _( msg = _(
"signing an image requires the --file option, " "signing an image requires the --file option, "
"passing files via stdin when signing is not " "passing files via stdin when signing is not "
@ -546,6 +554,10 @@ class CreateImage(command.ShowOne):
kwargs['img_signature_key_type'] = signer.padding_method kwargs['img_signature_key_type'] = signer.padding_method
image = image_client.create_image(**kwargs) image = image_client.create_image(**kwargs)
if parsed_args.filename:
fp.close()
return _format_image(image) return _format_image(image)
def _take_action_volume(self, parsed_args): def _take_action_volume(self, parsed_args):
@ -980,6 +992,7 @@ class SaveImage(command.Command):
parser.add_argument( parser.add_argument(
"--file", "--file",
metavar="<filename>", metavar="<filename>",
dest="filename",
help=_("Downloaded image save filename (default: stdout)"), help=_("Downloaded image save filename (default: stdout)"),
) )
parser.add_argument( parser.add_argument(
@ -993,7 +1006,7 @@ class SaveImage(command.Command):
image_client = self.app.client_manager.image image_client = self.app.client_manager.image
image = image_client.find_image(parsed_args.image) image = image_client.find_image(parsed_args.image)
output_file = parsed_args.file output_file = parsed_args.filename
if output_file is None: if output_file is None:
output_file = getattr(sys.stdout, "buffer", sys.stdout) output_file = getattr(sys.stdout, "buffer", sys.stdout)

94
openstackclient/tests/unit/image/v2/test_image.py
View File

@ -14,7 +14,6 @@
import copy import copy
import io import io
import os
import tempfile import tempfile
from unittest import mock from unittest import mock
@ -104,15 +103,8 @@ class TestImageCreate(TestImage):
disk_format=image.DEFAULT_DISK_FORMAT, disk_format=image.DEFAULT_DISK_FORMAT,
) )
# Verify update() was not called, if it was show the args self.assertEqual(self.expected_columns, columns)
self.assertEqual(self.client.update_image.call_args_list, []) self.assertCountEqual(self.expected_data, data)
self.assertEqual(
self.expected_columns,
columns)
self.assertCountEqual(
self.expected_data,
data)
@mock.patch('sys.stdin', side_effect=[None]) @mock.patch('sys.stdin', side_effect=[None])
def test_image_reserve_options(self, raw_input): def test_image_reserve_options(self, raw_input):
@ -121,10 +113,11 @@ class TestImageCreate(TestImage):
'--disk-format', 'ami', '--disk-format', 'ami',
'--min-disk', '10', '--min-disk', '10',
'--min-ram', '4', '--min-ram', '4',
('--protected' '--protected' if self.new_image.is_protected else '--unprotected',
if self.new_image.is_protected else '--unprotected'), (
('--private' '--private'
if self.new_image.visibility == 'private' else '--public'), if self.new_image.visibility == 'private' else '--public'
),
'--project', self.new_image.owner_id, '--project', self.new_image.owner_id,
'--project-domain', self.domain.id, '--project-domain', self.domain.id,
self.new_image.name, self.new_image.name,
@ -160,12 +153,8 @@ class TestImageCreate(TestImage):
visibility=self.new_image.visibility, visibility=self.new_image.visibility,
) )
self.assertEqual( self.assertEqual(self.expected_columns, columns)
self.expected_columns, self.assertCountEqual(self.expected_data, data)
columns)
self.assertCountEqual(
self.expected_data,
data)
def test_image_create_with_unexist_project(self): def test_image_create_with_unexist_project(self):
self.project_mock.get.side_effect = exceptions.NotFound(None) self.project_mock.get.side_effect = exceptions.NotFound(None)
@ -217,7 +206,7 @@ class TestImageCreate(TestImage):
self.new_image.name, self.new_image.name,
] ]
verifylist = [ verifylist = [
('file', imagefile.name), ('filename', imagefile.name),
('is_protected', self.new_image.is_protected), ('is_protected', self.new_image.is_protected),
('visibility', self.new_image.visibility), ('visibility', self.new_image.visibility),
('properties', {'Alpha': '1', 'Beta': '2'}), ('properties', {'Alpha': '1', 'Beta': '2'}),
@ -252,12 +241,12 @@ class TestImageCreate(TestImage):
self.expected_data, self.expected_data,
data) data)
@mock.patch('openstackclient.image.v2.image.get_data_file') @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
def test_image_create__progress_ignore_with_stdin( def test_image_create__progress_ignore_with_stdin(
self, mock_get_data_file, self, mock_get_data_from_stdin,
): ):
fake_stdin = io.StringIO('fake-image-data') fake_stdin = io.StringIO('fake-image-data')
mock_get_data_file.return_value = (fake_stdin, None) mock_get_data_from_stdin.return_value = fake_stdin
arglist = [ arglist = [
'--progress', '--progress',
@ -322,11 +311,11 @@ class TestImageCreate(TestImage):
) )
@mock.patch('osc_lib.utils.find_resource') @mock.patch('osc_lib.utils.find_resource')
@mock.patch('openstackclient.image.v2.image.get_data_file') @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
def test_image_create_from_volume(self, mock_get_data_f, mock_get_vol): def test_image_create_from_volume(self, mock_get_data_f, mock_get_vol):
fake_vol_id = 'fake-volume-id' fake_vol_id = 'fake-volume-id'
mock_get_data_f.return_value = (None, None) mock_get_data_f.return_value = None
class FakeVolume: class FakeVolume:
id = fake_vol_id id = fake_vol_id
@ -353,12 +342,12 @@ class TestImageCreate(TestImage):
) )
@mock.patch('osc_lib.utils.find_resource') @mock.patch('osc_lib.utils.find_resource')
@mock.patch('openstackclient.image.v2.image.get_data_file') @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
def test_image_create_from_volume_fail(self, mock_get_data_f, def test_image_create_from_volume_fail(self, mock_get_data_f,
mock_get_vol): mock_get_vol):
fake_vol_id = 'fake-volume-id' fake_vol_id = 'fake-volume-id'
mock_get_data_f.return_value = (None, None) mock_get_data_f.return_value = None
class FakeVolume: class FakeVolume:
id = fake_vol_id id = fake_vol_id
@ -379,7 +368,7 @@ class TestImageCreate(TestImage):
parsed_args) parsed_args)
@mock.patch('osc_lib.utils.find_resource') @mock.patch('osc_lib.utils.find_resource')
@mock.patch('openstackclient.image.v2.image.get_data_file') @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
def test_image_create_from_volume_v31(self, mock_get_data_f, def test_image_create_from_volume_v31(self, mock_get_data_f,
mock_get_vol): mock_get_vol):
@ -387,7 +376,7 @@ class TestImageCreate(TestImage):
api_versions.APIVersion('3.1')) api_versions.APIVersion('3.1'))
fake_vol_id = 'fake-volume-id' fake_vol_id = 'fake-volume-id'
mock_get_data_f.return_value = (None, None) mock_get_data_f.return_value = None
class FakeVolume: class FakeVolume:
id = fake_vol_id id = fake_vol_id
@ -1798,7 +1787,7 @@ class TestImageSave(TestImage):
arglist = ['--file', '/path/to/file', self.image.id] arglist = ['--file', '/path/to/file', self.image.id]
verifylist = [ verifylist = [
('file', '/path/to/file'), ('filename', '/path/to/file'),
('image', self.image.id), ('image', self.image.id),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1813,49 +1802,26 @@ class TestImageSave(TestImage):
class TestImageGetData(TestImage): class TestImageGetData(TestImage):
def setUp(self): def test_get_data_from_stdin(self):
super().setUp() fd = io.BytesIO(b"some initial binary data: \x00\x01")
self.args = mock.Mock()
def test_get_data_file_file(self):
(fd, fname) = tempfile.mkstemp(prefix='osc_test_image')
self.args.file = fname
(test_fd, test_name) = image.get_data_file(self.args)
self.assertEqual(fname, test_name)
test_fd.close()
os.unlink(fname)
def test_get_data_file_2(self):
self.args.file = None
f = io.BytesIO(b"some initial binary data: \x00\x01")
with mock.patch('sys.stdin') as stdin: with mock.patch('sys.stdin') as stdin:
stdin.return_value = f stdin.return_value = fd
stdin.isatty.return_value = False stdin.isatty.return_value = False
stdin.buffer = f stdin.buffer = fd
(test_fd, test_name) = image.get_data_file(self.args) test_fd = image.get_data_from_stdin()
# Ensure data written to temp file is correct # Ensure data written to temp file is correct
self.assertEqual(f, test_fd) self.assertEqual(fd, test_fd)
self.assertIsNone(test_name)
def test_get_data_file_3(self): def test_get_data_from_stdin__interactive(self):
fd = io.BytesIO(b"some initial binary data: \x00\x01")
self.args.file = None
f = io.BytesIO(b"some initial binary data: \x00\x01")
with mock.patch('sys.stdin') as stdin: with mock.patch('sys.stdin') as stdin:
# There is stdin, but interactive # There is stdin, but interactive
stdin.return_value = f stdin.return_value = fd
(test_fd, test_fname) = image.get_data_file(self.args) test_fd = image.get_data_from_stdin()
self.assertIsNone(test_fd) self.assertIsNone(test_fd)
self.assertIsNone(test_fname)