From e8038fa26ea64a4164a84143f8a1a07782dfd938 Mon Sep 17 00:00:00 2001 From: Mitchell Hentges Date: Wed, 10 Jul 2019 16:13:52 -0700 Subject: [PATCH 1/2] Splits read-only and writable transaction logic --- CHANGELOG.md | 5 + mozapkpublisher/check_rollout.py | 17 +- mozapkpublisher/common/exceptions.py | 7 - mozapkpublisher/common/googleplay.py | 231 ++++++++++-------- mozapkpublisher/push_apk.py | 44 +--- .../test/common/test_googleplay.py | 194 +++++++-------- mozapkpublisher/test/test_check_rollout.py | 20 +- mozapkpublisher/test/test_push_apk.py | 38 +-- .../test/test_update_apk_description.py | 11 +- mozapkpublisher/update_apk_description.py | 17 +- 10 files changed, 288 insertions(+), 296 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae02137..cfcafa3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [upcoming] + +### Changed +* Separates read-only logic from logic that requires a transaction + ## [4.1.0] - 2019-07-10 ### Removed * `--skip-check-package-names`. When pushing or checking an APK, expected package names must always be provided diff --git a/mozapkpublisher/check_rollout.py b/mozapkpublisher/check_rollout.py index ad9b8e9a..b54de5a0 100755 --- a/mozapkpublisher/check_rollout.py +++ b/mozapkpublisher/check_rollout.py @@ -8,17 +8,17 @@ import requests from argparse import ArgumentParser -from mozapkpublisher.common.googleplay import EditService, add_general_google_play_arguments - +from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \ + ReadOnlyGooglePlay, GooglePlayConnection DAY = 24 * 60 * 60 logger = logging.getLogger(__name__) -def check_rollout(edit_service, days): +def check_rollout(google_play, days): """Check if package_name has a release on staged rollout for too long""" - track_status = edit_service.get_track_status(track='production') + track_status = google_play.get_track_status(track='production') releases = track_status['releases'] for release in releases: if release['status'] == 'inProgress': @@ -40,12 +40,11 @@ def main(): type=int, default=7) config = parser.parse_args() - edit_service = EditService( + google_play = ReadOnlyGooglePlay.create(GooglePlayConnection.open( config.service_account, - config.google_play_credentials_file.name, - package_name='org.mozilla.firefox', - ) - for (release, age) in check_rollout(edit_service, config.days): + config.google_play_credentials_file.name + ), 'org.mozilla.firefox') + for (release, age) in check_rollout(google_play, config.days): print('fennec {} is on staged rollout at {}% but it shipped {} days ago'.format( release['name'], int(release['userFraction'] * 100), int(age / DAY))) diff --git a/mozapkpublisher/common/exceptions.py b/mozapkpublisher/common/exceptions.py index a1a15150..6c6b1e8a 100644 --- a/mozapkpublisher/common/exceptions.py +++ b/mozapkpublisher/common/exceptions.py @@ -21,13 +21,6 @@ def __init__(self, checked_file, expected, actual): ) -class NoTransactionError(LoggedError): - def __init__(self, package_name): - super(NoTransactionError, self).__init__( - 'Transaction has not been started for package "{}"'.format(package_name) - ) - - class NotMultiLocaleApk(LoggedError): def __init__(self, apk_path, unique_locales): super(NotMultiLocaleApk, self).__init__( diff --git a/mozapkpublisher/common/googleplay.py b/mozapkpublisher/common/googleplay.py index b2e7f22e..fd9337f4 100644 --- a/mozapkpublisher/common/googleplay.py +++ b/mozapkpublisher/common/googleplay.py @@ -13,6 +13,8 @@ """ import argparse +from contextlib import contextmanager + import httplib2 import json import logging @@ -23,7 +25,7 @@ # HACK: importing mock in production is useful for option `--do-not-contact-google-play` from unittest.mock import MagicMock -from mozapkpublisher.common.exceptions import NoTransactionError, WrongArgumentGiven +from mozapkpublisher.common.exceptions import WrongArgumentGiven logger = logging.getLogger(__name__) @@ -41,57 +43,118 @@ def add_general_google_play_arguments(parser): --service-account and --credentials must still be provided (you can just fill them with random string and file).''') -class EditService(object): - def __init__(self, service_account, credentials_file_path, package_name, commit=False, contact_google_play=True): - self._contact_google_play = contact_google_play - if self._contact_google_play: - general_service = _connect(service_account, credentials_file_path) - self._service = general_service.edits() - else: - self._service = _craft_google_play_service_mock() - logger.warning('`--do-not-contact-google-play` option was given. Not a single request to Google Play will be made!') +class GooglePlayConnection: + def __init__(self, edit_resource): + self._edit_resource = edit_resource - self._package_name = package_name - self._commit = commit - self.start_new_transaction() - - def start_new_transaction(self): - result = self._service.insert(body={}, packageName=self._package_name).execute() - self._edit_id = result['id'] - - def transaction_required(method): - def _transaction_required(*args, **kwargs): - edit_service = args[0] - if edit_service._edit_id is None: - raise NoTransactionError(edit_service._package_name) - - return method(*args, **kwargs) - return _transaction_required - - @transaction_required - def commit_transaction(self): - if self._commit: - self._service.commit(editId=self._edit_id, packageName=self._package_name).execute() - logger.info('Changes committed') - logger.debug('edit_id "{}" for package "{}" has been committed'.format(self._edit_id, self._package_name)) - else: - logger.warning('`commit` option was not given. Transaction not committed.') + def get_edit_resource(self): + return self._edit_resource + + @staticmethod + def open(service_account, credentials_file_path): + # Create an httplib2.Http object to handle our HTTP requests an + # authorize it with the Credentials. Note that the first parameter, + # service_account_name, is the Email address created for the Service + # account. It must be the email address associated with the key that + # was created. + scope = 'https://www.googleapis.com/auth/androidpublisher' + credentials = ServiceAccountCredentials.from_p12_keyfile( + service_account, + credentials_file_path, + scopes=scope + ) + http = httplib2.Http() + http = credentials.authorize(http) + + service = build(serviceName='androidpublisher', version='v3', http=http, + cache_discovery=False) + + return GooglePlayConnection(service.edits()) + + +class _ExecuteDummy: + def __init__(self, return_value): + self._return_value = return_value + + def execute(self): + return self._return_value + + +class MockGooglePlayConnection: + @staticmethod + def get_edit_resource(): + edit_service_mock = MagicMock() + + edit_service_mock.insert = lambda *args, **kwargs: _ExecuteDummy( + {'id': 'fake-transaction-id'}) + edit_service_mock.commit = lambda *args, **kwargs: _ExecuteDummy(None) + + apks_mock = MagicMock() + apks_mock.upload = lambda *args, **kwargs: _ExecuteDummy( + {'versionCode': 'fake-version-code'}) + edit_service_mock.apks = lambda *args, **kwargs: apks_mock + + update_mock = MagicMock() + update_mock.update = lambda *args, **kwargs: _ExecuteDummy('fake-update-response') + edit_service_mock.tracks = lambda *args, **kwargs: update_mock + edit_service_mock.listings = lambda *args, **kwargs: update_mock + edit_service_mock.apklistings = lambda *args, **kwargs: update_mock + + return edit_service_mock - self._edit_id = None - @transaction_required +def connection_for_options(contact_google_play, service_account, credentials_file): + if contact_google_play: + return GooglePlayConnection.open(service_account, credentials_file.name) + else: + logger.warning('Not a single request to Google Play will be made, since `contact_google_play` was set') + return MockGooglePlayConnection() + + +class ReadOnlyGooglePlay: + """Read-only access to the Google Play store + + Create an instance by calling ReadOnlyGooglePlay.create() instead of using the constructor + """ + + def __init__(self, edit_resource, edit_id, package_name): + self._edit_resource = edit_resource + self._edit_id = edit_id + self._package_name = package_name + def get_track_status(self, track): - response = self._service.tracks().get( - editId=self._edit_id, track=track, packageName=self._package_name + response = self._edit_resource.tracks().get( + editId=self._edit_id, + track=track, + packageName=self._package_name ).execute() - logger.debug(u'Track "{}" has status: {}'.format(track, response)) + logger.debug('Track "{}" has status: {}'.format(track, response)) return response - @transaction_required + @staticmethod + def create(connection, package_name): + edit_resource = connection.get_edit_resource() + edit_id = edit_resource.insert(body={}, packageName=package_name).execute()['id'] + return ReadOnlyGooglePlay(edit_resource, edit_id, package_name) + + +class WritableGooglePlay(ReadOnlyGooglePlay): + """Read-write access to the Google Play store + + Create an instance by calling WritableGooglePlay.transaction(), instead of using the + constructor. This will automatically handle committing the transaction when the "with" block + ends. + + E.g.: `with WritableGooglePlay.transaction() as google_play:` + """ + + def __init__(self, edit_resource, edit_id, package_name): + super().__init__(edit_resource, edit_id, package_name) + def upload_apk(self, apk_path): logger.info('Uploading "{}" ...'.format(apk_path)) try: - response = self._service.apks().upload( + response = self._edit_resource.apks().upload( editId=self._edit_id, packageName=self._package_name, media_body=apk_path @@ -103,19 +166,17 @@ def upload_apk(self, apk_path): # XXX This is really how data is returned by the googleapiclient. error_content = json.loads(e.content) errors = error_content['error']['errors'] - if ( - len(errors) == 1 and - errors[0]['reason'] in ( - 'apkUpgradeVersionConflict', 'apkNotificationMessageKeyUpgradeVersionConflict' - ) - ): + if (len(errors) == 1 and errors[0]['reason'] in ( + 'apkUpgradeVersionConflict', + 'apkNotificationMessageKeyUpgradeVersionConflict' + )): logger.warning( - 'APK "{}" has already been uploaded on Google Play. Skipping...'.format(apk_path) + 'APK "{}" has already been uploaded on Google Play. Skipping...'.format( + apk_path) ) return raise - @transaction_required def update_track(self, track, version_codes, rollout_percentage=None): body = { u'releases': [{ @@ -125,32 +186,32 @@ def update_track(self, track, version_codes, rollout_percentage=None): } if rollout_percentage is not None: if rollout_percentage < 0 or rollout_percentage > 100: - raise WrongArgumentGiven('rollout percentage must be between 0 and 100. Value given: {}'.format(rollout_percentage)) + raise WrongArgumentGiven( + 'rollout percentage must be between 0 and 100. Value given: {}'.format( + rollout_percentage)) body[u'userFraction'] = rollout_percentage / 100.0 # Ensure float in Python 2 - response = self._service.tracks().update( + response = self._edit_resource.tracks().update( editId=self._edit_id, track=track, packageName=self._package_name, body=body ).execute() logger.info('Track "{}" updated with: {}'.format(track, body)) logger.debug('Track update response: {}'.format(response)) - @transaction_required def update_listings(self, language, title, full_description, short_description): body = { 'fullDescription': full_description, 'shortDescription': short_description, 'title': title, } - response = self._service.listings().update( + response = self._edit_resource.listings().update( editId=self._edit_id, packageName=self._package_name, language=language, body=body ).execute() logger.info(u'Listing for language "{}" has been updated with: {}'.format(language, body)) logger.debug(u'Listing response: {}'.format(response)) - @transaction_required def update_whats_new(self, language, apk_version_code, whats_new): - response = self._service.apklistings().update( + response = self._edit_resource.apklistings().update( editId=self._edit_id, packageName=self._package_name, language=language, apkVersionCode=apk_version_code, body={'recentChanges': whats_new} ).execute() @@ -159,48 +220,16 @@ def update_whats_new(self, language, apk_version_code, whats_new): )) logger.debug(u'Apk listing response: {}'.format(response)) - -def _craft_google_play_service_mock(): - edit_service_mock = MagicMock() - - edit_service_mock.insert = lambda *args, **kwargs: _ExecuteDummy({'id': 'fake-transaction-id'}) - edit_service_mock.commit = lambda *args, **kwargs: _ExecuteDummy(None) - - apks_mock = MagicMock() - apks_mock.upload = lambda *args, **kwargs: _ExecuteDummy({'versionCode': 'fake-version-code'}) - edit_service_mock.apks = lambda *args, **kwargs: apks_mock - - update_mock = MagicMock() - update_mock.update = lambda *args, **kwargs: _ExecuteDummy('fake-update-response') - edit_service_mock.tracks = lambda *args, **kwargs: update_mock - edit_service_mock.listings = lambda *args, **kwargs: update_mock - edit_service_mock.apklistings = lambda *args, **kwargs: update_mock - - return edit_service_mock - - -class _ExecuteDummy(): - def __init__(self, return_value): - self._return_value = return_value - - def execute(self): - return self._return_value - - -def _connect(service_account, credentials_file_path): - """ Connect to the google play interface - """ - - # Create an httplib2.Http object to handle our HTTP requests an - # authorize it with the Credentials. Note that the first parameter, - # service_account_name, is the Email address created for the Service - # account. It must be the email address associated with the key that - # was created. - scope = 'https://www.googleapis.com/auth/androidpublisher' - credentials = ServiceAccountCredentials.from_p12_keyfile(service_account, credentials_file_path, scopes=scope) - http = httplib2.Http() - http = credentials.authorize(http) - - service = build(serviceName='androidpublisher', version='v3', http=http, cache_discovery=False) - - return service + @staticmethod + @contextmanager + def transaction(connection, package_name, do_not_commit=False): + edit_resource = connection.get_edit_resource() + edit_id = edit_resource.insert(body={}, packageName=package_name).execute()['id'] + google_play = WritableGooglePlay(edit_resource, edit_id, package_name) + yield google_play + if do_not_commit: + logger.warning('Transaction not committed, since `do_not_commit` was set') + else: + edit_resource.commit(editId=edit_id, packageName=package_name) + logger.info('Changes committed') + logger.debug('edit_id "{}" for "{}" has been committed'.format(edit_id, package_name)) diff --git a/mozapkpublisher/push_apk.py b/mozapkpublisher/push_apk.py index 7e11d067..05c8698d 100755 --- a/mozapkpublisher/push_apk.py +++ b/mozapkpublisher/push_apk.py @@ -6,6 +6,7 @@ from mozapkpublisher.common import googleplay, main_logging from mozapkpublisher.common.apk import add_apk_checks_arguments, extract_and_check_apks_metadata from mozapkpublisher.common.exceptions import WrongArgumentGiven +from mozapkpublisher.common.googleplay import WritableGooglePlay, connection_for_options logger = logging.getLogger(__name__) @@ -62,43 +63,22 @@ def push_apk( skip_check_ordered_version_codes, ) + # TODO make programmatic usage of this library provide a "GooglePlayConnection" object, rather + # than having to provide redundant information like "service_account" and "credentials" when + # "contact_google_play" is false + connection = connection_for_options(contact_google_play, service_account, google_play_credentials_file) + # Each distinct product must be uploaded in different Google Play transaction, so we split them # by package name here. split_apk_metadata = _split_apk_metadata_per_package_name(apks_metadata_per_paths) - for (package_name, apks_metadata) in split_apk_metadata.items(): - _upload_apks( - service_account, - google_play_credentials_file, - commit, - contact_google_play, - apks_metadata, - package_name, - track, - rollout_percentage, - ) - - -def _upload_apks( - service_account, - google_play_credentials_file, - commit, - contact_google_play, - apks_metadata_per_paths, - package_name, - track, - rollout_percentage, -): - edit_service = googleplay.EditService( - service_account, google_play_credentials_file.name, package_name, commit, contact_google_play - ) - - for path, metadata in apks_metadata_per_paths.items(): - edit_service.upload_apk(path) + with WritableGooglePlay.transaction(connection, package_name, + do_not_commit=not commit) as google_play: + for path, metadata in apks_metadata_per_paths.items(): + google_play.upload_apk(path) - all_version_codes = _get_ordered_version_codes(apks_metadata_per_paths) - edit_service.update_track(track, all_version_codes, rollout_percentage) - edit_service.commit_transaction() + all_version_codes = _get_ordered_version_codes(apks_metadata_per_paths) + google_play.update_track(track, all_version_codes, rollout_percentage) def _split_apk_metadata_per_package_name(apks_metadata_per_paths): diff --git a/mozapkpublisher/test/common/test_googleplay.py b/mozapkpublisher/test/common/test_googleplay.py index ec8b96f1..e99509be 100644 --- a/mozapkpublisher/test/common/test_googleplay.py +++ b/mozapkpublisher/test/common/test_googleplay.py @@ -1,5 +1,7 @@ import argparse import json + +from mock import ANY, patch import pytest import random import tempfile @@ -7,8 +9,11 @@ from googleapiclient.errors import HttpError from unittest.mock import MagicMock -from mozapkpublisher.common.exceptions import NoTransactionError, WrongArgumentGiven -from mozapkpublisher.common.googleplay import add_general_google_play_arguments, EditService +from mozapkpublisher.common import googleplay +from mozapkpublisher.common.exceptions import WrongArgumentGiven +from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \ + WritableGooglePlay, MockGooglePlayConnection, ReadOnlyGooglePlay, GooglePlayConnection, \ + connection_for_options from mozapkpublisher.test import does_not_raise @@ -25,75 +30,61 @@ def test_add_general_google_play_arguments(): assert config.service_account == 'dummy@dummy' -def set_up_edit_service_mock(_monkeypatch): - general_service_mock = MagicMock() - edit_service_mock = MagicMock() - new_transaction_mock = MagicMock() - - new_transaction_mock.execute = lambda: {'id': random.randint(0, 1000)} - edit_service_mock.insert = lambda body, packageName: new_transaction_mock - general_service_mock.edits = lambda: edit_service_mock +@patch.object(googleplay, 'MockGooglePlayConnection') +def test_connection_for_options_contact(mock): + connection_for_options(False, '', MagicMock) + mock.assert_called_with() - _monkeypatch.setattr('mozapkpublisher.common.googleplay._connect', lambda _, __: general_service_mock) - return edit_service_mock +@patch.object(googleplay.ServiceAccountCredentials, 'from_p12_keyfile') +@patch.object(googleplay, 'build') +def test_google_play_connection(mock_build, _): + mock_service = MagicMock() + mock_service.edits.return_value = 'edit resource' + mock_build.return_value = mock_service + assert GooglePlayConnection.open('service_account', 'file').get_edit_resource() == 'edit resource' -def test_edit_service_starts_new_transaction_upon_init(monkeypatch): - set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - edit_service.upload_apk(apk_path='/path/to/dummy.apk') +@pytest.fixture +def edit_resource_mock(): + edit_resource = MagicMock() + new_transaction_mock = MagicMock() -def test_edit_service_raises_error_if_no_transaction_started(monkeypatch): - set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - edit_service.commit_transaction() - with pytest.raises(NoTransactionError): - edit_service.upload_apk(apk_path='/path/to/dummy.apk') + new_transaction_mock.execute = lambda: {'id': random.randint(0, 1000)} + edit_resource.insert = lambda body, packageName: new_transaction_mock + return edit_resource -def test_edit_service_starts_new_transaction_manually(monkeypatch): - set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - old_edit_id = edit_service._edit_id - edit_service.commit_transaction() - edit_service.start_new_transaction() +def test_read_only_google_play_no_commit_transaction(): + connection = MockGooglePlayConnection() + mock_edits_resource = MagicMock() + connection.get_edit_resource = lambda: mock_edits_resource + ReadOnlyGooglePlay.create(connection, 'package.name') - assert edit_service._edit_id != old_edit_id + mock_edits_resource.commit.assert_not_called() -def test_edit_service_commits_only_when_option_is_provided(monkeypatch): - edit_service_mock = set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - edit_service.commit_transaction() - edit_service_mock.commit.assert_not_called() +def test_writable_google_play_commit_transaction(): + connection = MockGooglePlayConnection() + mock_edits_resource = MagicMock() + connection.get_edit_resource = lambda: mock_edits_resource + with WritableGooglePlay.transaction(connection, 'package.name') as _: + pass - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name', commit=True) - current_edit_id = edit_service._edit_id - edit_service.commit_transaction() - edit_service_mock.commit.assert_called_once_with(editId=current_edit_id, packageName='dummy_package_name') + mock_edits_resource.commit.assert_called_with(editId=ANY, packageName='package.name') -def test_edit_service_is_allowed_to_not_make_a_single_call_to_google_play(monkeypatch): - edit_service_mock = set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name', commit=True, contact_google_play=False) - edit_service.upload_apk(apk_path='/path/to/dummy.apk') - edit_service.update_listings( - language='some_language', title='some_title', full_description='some_description', short_description='some_desc' - ) - edit_service.update_track(track='some_track', version_codes=['1', '2']) - edit_service.update_whats_new(language='some_language', apk_version_code='some_version_code', whats_new='some_text') - edit_service.commit_transaction() +def test_writable_google_play_argument_do_not_commit_transaction(): + connection = MockGooglePlayConnection() + mock_edits_resource = MagicMock() + connection.get_edit_resource = lambda: mock_edits_resource + with WritableGooglePlay.transaction(connection, 'package.name', do_not_commit=True) as _: + pass - edit_service_mock.apks().upload.assert_not_called() - edit_service_mock.apklistings().update.assert_not_called() - edit_service_mock.tracks().update.assert_not_called() - edit_service_mock.apklistings().update.assert_not_called() - edit_service_mock.commit.assert_not_called() + mock_edits_resource.commit.assert_not_called() -def test_get_track_status(monkeypatch): - edit_mock = set_up_edit_service_mock(monkeypatch) +def test_get_track_status(edit_resource_mock): release_data = { "releases": [{ "name": "61.0", @@ -113,49 +104,47 @@ def test_get_track_status(monkeypatch): }], } - edit_mock.tracks().get().execute.return_value = release_data + edit_resource_mock.tracks().get().execute.return_value = release_data - edit_mock.tracks().get.reset_mock() + edit_resource_mock.tracks().get.reset_mock() - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - assert edit_service.get_track_status(track='production') == release_data - edit_mock.tracks().get.assert_called_once_with( - editId=edit_service._edit_id, + google_play = ReadOnlyGooglePlay(edit_resource_mock, 1, 'dummy_package_name') + assert google_play.get_track_status(track='production') == release_data + edit_resource_mock.tracks().get.assert_called_once_with( + editId=1, track='production', packageName='dummy_package_name', ) -def test_upload_apk_returns_files_metadata(monkeypatch): - edit_mock = set_up_edit_service_mock(monkeypatch) - edit_mock.apks().upload().execute.return_value = { +def test_upload_apk_returns_files_metadata(edit_resource_mock): + edit_resource_mock.apks().upload().execute.return_value = { 'binary': {'sha1': '1234567890abcdef1234567890abcdef12345678'}, 'versionCode': 2015012345 } - edit_mock.apks().upload.reset_mock() + edit_resource_mock.apks().upload.reset_mock() - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') - edit_service.upload_apk(apk_path='/path/to/dummy.apk') - edit_mock.apks().upload.assert_called_once_with( - editId=edit_service._edit_id, + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') + google_play.upload_apk(apk_path='/path/to/dummy.apk') + edit_resource_mock.apks().upload.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', media_body='/path/to/dummy.apk', ) @pytest.mark.parametrize('http_status_code', (400, 403)) -def test_upload_apk_errors_out(monkeypatch, http_status_code): - edit_mock = set_up_edit_service_mock(monkeypatch) - edit_mock.apks().upload().execute.side_effect = HttpError( +def test_upload_apk_errors_out(edit_resource_mock, http_status_code): + edit_resource_mock.apks().upload().execute.side_effect = HttpError( # XXX status is presented as a string by googleapiclient resp={'status': str(http_status_code)}, # XXX content must be bytes # https://github.com/googleapis/google-api-python-client/blob/ffea1a7fe9d381d23ab59048263c631cc2b45323/googleapiclient/errors.py#L41 content=b'{"error": {"errors": [{"reason": "someRandomReason"}] } }', ) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') with pytest.raises(HttpError): - edit_service.upload_apk(apk_path='/path/to/dummy.apk') + google_play.upload_apk(apk_path='/path/to/dummy.apk') @pytest.mark.parametrize('reason, expectation', ( @@ -163,8 +152,7 @@ def test_upload_apk_errors_out(monkeypatch, http_status_code): ('apkNotificationMessageKeyUpgradeVersionConflict', does_not_raise()), ('someRandomReason', pytest.raises(HttpError)), )) -def test_upload_apk_does_not_error_out_when_apk_is_already_published(monkeypatch, reason, expectation): - edit_mock = set_up_edit_service_mock(monkeypatch) +def test_upload_apk_does_not_error_out_when_apk_is_already_published(edit_resource_mock, reason, expectation): content = { 'error': { 'errors': [{ @@ -176,24 +164,23 @@ def test_upload_apk_does_not_error_out_when_apk_is_already_published(monkeypatch # https://github.com/googleapis/google-api-python-client/blob/ffea1a7fe9d381d23ab59048263c631cc2b45323/googleapiclient/errors.py#L41 content_bytes = json.dumps(content).encode('ascii') - edit_mock.apks().upload().execute.side_effect = HttpError( + edit_resource_mock.apks().upload().execute.side_effect = HttpError( # XXX status is presented as a string by googleapiclient resp={'status': '403'}, content=content_bytes, ) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') with expectation: - edit_service.upload_apk(apk_path='/path/to/dummy.apk') + google_play.upload_apk(apk_path='/path/to/dummy.apk') -def test_update_track(monkeypatch): - edit_mock = set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') +def test_update_track(edit_resource_mock): + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - edit_service.update_track('alpha', ['2015012345', '2015012347']) - edit_mock.tracks().update.assert_called_once_with( - editId=edit_service._edit_id, + google_play.update_track('alpha', ['2015012345', '2015012347']) + edit_resource_mock.tracks().update.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', track='alpha', body={ @@ -204,10 +191,10 @@ def test_update_track(monkeypatch): }, ) - edit_mock.tracks().update.reset_mock() - edit_service.update_track('rollout', ['2015012345', '2015012347'], rollout_percentage=1) - edit_mock.tracks().update.assert_called_once_with( - editId=edit_service._edit_id, + edit_resource_mock.tracks().update.reset_mock() + google_play.update_track('rollout', ['2015012345', '2015012347'], rollout_percentage=1) + edit_resource_mock.tracks().update.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', track='rollout', body={ @@ -221,26 +208,24 @@ def test_update_track(monkeypatch): @pytest.mark.parametrize('invalid_percentage', (-1, 101)) -def test_update_track_should_refuse_wrong_percentage(monkeypatch, invalid_percentage): - set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') +def test_update_track_should_refuse_wrong_percentage(edit_resource_mock, invalid_percentage): + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') with pytest.raises(WrongArgumentGiven): - edit_service.update_track('rollout', ['2015012345', '2015012347'], invalid_percentage) + google_play.update_track('rollout', ['2015012345', '2015012347'], invalid_percentage) -def test_update_listings(monkeypatch): - edit_mock = set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') +def test_update_listings(edit_resource_mock): + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - edit_service.update_listings( + google_play.update_listings( 'en-GB', title='Firefox for Android Beta', full_description='Long description', short_description='Short', ) - edit_mock.listings().update.assert_called_once_with( - editId=edit_service._edit_id, + edit_resource_mock.listings().update.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', language='en-GB', body={ @@ -251,13 +236,12 @@ def test_update_listings(monkeypatch): ) -def test_update_whats_new(monkeypatch): - edit_mock = set_up_edit_service_mock(monkeypatch) - edit_service = EditService('service_account', 'credentials_file_path', 'dummy_package_name') +def test_update_whats_new(edit_resource_mock): + google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - edit_service.update_whats_new('en-GB', '2015012345', 'Check out this cool feature!') - edit_mock.apklistings().update.assert_called_once_with( - editId=edit_service._edit_id, + google_play.update_whats_new('en-GB', '2015012345', 'Check out this cool feature!') + edit_resource_mock.apklistings().update.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', language='en-GB', apkVersionCode='2015012345', diff --git a/mozapkpublisher/test/test_check_rollout.py b/mozapkpublisher/test/test_check_rollout.py index 7dccea93..5d16ebb3 100644 --- a/mozapkpublisher/test/test_check_rollout.py +++ b/mozapkpublisher/test/test_check_rollout.py @@ -21,9 +21,9 @@ def set_up_mocks(_requests_mock, tracks): _requests_mock.head('https://archive.mozilla.org/pub/mobile/releases/{}/SHA512SUMS'.format('62.0'), status_code=404) - edit_service_mock = create_autospec(googleplay.EditService) - edit_service_mock.get_track_status.return_value = tracks - return edit_service_mock + google_play_mock = create_autospec(googleplay.ReadOnlyGooglePlay) + google_play_mock.get_track_status.return_value = tracks + return google_play_mock def test_new_rollout(requests_mock): @@ -47,12 +47,12 @@ def test_new_rollout(requests_mock): }], } - edit_service_mock = set_up_mocks(requests_mock, tracks) + google_play_mock = set_up_mocks(requests_mock, tracks) with pytest.raises(StopIteration): - next(check_rollout.check_rollout(edit_service_mock, 7)) + next(check_rollout.check_rollout(google_play_mock, 7)) - gen = check_rollout.check_rollout(edit_service_mock, .5) + gen = check_rollout.check_rollout(google_play_mock, .5) release, age = next(gen) assert release['name'] == '61.0' assert age >= check_rollout.DAY @@ -75,9 +75,9 @@ def test_old_rollout(requests_mock): }], } - edit_service_mock = set_up_mocks(requests_mock, tracks) + google_play_mock = set_up_mocks(requests_mock, tracks) - gen = check_rollout.check_rollout(edit_service_mock, 7) + gen = check_rollout.check_rollout(google_play_mock, 7) release, age = next(gen) assert release['name'] == '60.0.2' assert age >= 10 * check_rollout.DAY @@ -100,7 +100,7 @@ def test_rc_rollout(requests_mock): }], } - edit_service_mock = set_up_mocks(requests_mock, tracks) + google_play_mock = set_up_mocks(requests_mock, tracks) with pytest.raises(StopIteration): - next(check_rollout.check_rollout(edit_service_mock, 7)) + next(check_rollout.check_rollout(google_play_mock, 7)) diff --git a/mozapkpublisher/test/test_push_apk.py b/mozapkpublisher/test/test_push_apk.py index 5861c52a..78212ca9 100644 --- a/mozapkpublisher/test/test_push_apk.py +++ b/mozapkpublisher/test/test_push_apk.py @@ -1,3 +1,5 @@ +from contextlib import contextmanager + import mozapkpublisher import os import pytest @@ -27,11 +29,11 @@ @pytest.fixture -def edit_service_mock(): - return create_autospec(googleplay.EditService) +def writable_google_play_mock(): + return create_autospec(googleplay.WritableGooglePlay) -def set_up_mocks(monkeypatch_, edit_service_mock_): +def set_up_mocks(monkeypatch_, writable_google_play_mock_): def _metadata(*args, **kwargs): return { apk_arm.name: { @@ -72,11 +74,15 @@ def _metadata(*args, **kwargs): } } - monkeypatch_.setattr(googleplay, 'EditService', lambda _, __, ___, commit, contact_google_play: edit_service_mock_) + @contextmanager + def fake_transaction(_, __, do_not_commit): + yield writable_google_play_mock_ + + monkeypatch_.setattr(googleplay.WritableGooglePlay, 'transaction', fake_transaction) monkeypatch_.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', _metadata) -def test_invalid_rollout_percentage(edit_service_mock, monkeypatch): +def test_invalid_rollout_percentage(): with pytest.raises(WrongArgumentGiven): # missing percentage push_apk(APKS, SERVICE_ACCOUNT, credentials, 'rollout', []) @@ -87,13 +93,13 @@ def test_invalid_rollout_percentage(edit_service_mock, monkeypatch): push_apk(APKS, SERVICE_ACCOUNT, credentials, invalid_track, [], rollout_percentage=valid_percentage) -def test_valid_rollout_percentage(edit_service_mock, monkeypatch): - set_up_mocks(monkeypatch, edit_service_mock) +def test_valid_rollout_percentage(writable_google_play_mock, monkeypatch): + set_up_mocks(monkeypatch, writable_google_play_mock) valid_percentage = 50 - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'rollout', [], rollout_percentage=valid_percentage) - edit_service_mock.update_track.assert_called_once_with('rollout', ['0', '1'], valid_percentage) - edit_service_mock.update_track.reset_mock() + push_apk(APKS, SERVICE_ACCOUNT, credentials, 'rollout', [], rollout_percentage=valid_percentage, contact_google_play=False) + writable_google_play_mock.update_track.assert_called_once_with('rollout', ['0', '1'], valid_percentage) + writable_google_play_mock.update_track.reset_mock() def test_get_ordered_version_codes(): @@ -107,16 +113,15 @@ def test_get_ordered_version_codes(): }) == ['0', '1'] # should be sorted -def test_upload_apk(edit_service_mock, monkeypatch): - set_up_mocks(monkeypatch, edit_service_mock) +def test_upload_apk(writable_google_play_mock, monkeypatch): + set_up_mocks(monkeypatch, writable_google_play_mock) - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'alpha', []) + push_apk(APKS, SERVICE_ACCOUNT, credentials, 'alpha', [], contact_google_play=False) for apk_file in (apk_arm, apk_x86): - edit_service_mock.upload_apk.assert_any_call(apk_file.name) + writable_google_play_mock.upload_apk.assert_any_call(apk_file.name) - edit_service_mock.update_track.assert_called_once_with('alpha', ['0', '1'], None) - edit_service_mock.commit_transaction.assert_called_once_with() + writable_google_play_mock.update_track.assert_called_once_with('alpha', ['0', '1'], None) def test_get_distinct_package_name_apk_metadata(): @@ -162,7 +167,6 @@ def test_push_apk_tunes_down_logs(monkeypatch): monkeypatch.setattr('mozapkpublisher.push_apk.main_logging', main_logging_mock) monkeypatch.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', MagicMock()) monkeypatch.setattr('mozapkpublisher.push_apk._split_apk_metadata_per_package_name', MagicMock()) - monkeypatch.setattr('mozapkpublisher.push_apk._upload_apks', MagicMock()) push_apk(APKS, SERVICE_ACCOUNT, credentials, 'alpha', [], contact_google_play=False) diff --git a/mozapkpublisher/test/test_update_apk_description.py b/mozapkpublisher/test/test_update_apk_description.py index f86a7136..d4516617 100644 --- a/mozapkpublisher/test/test_update_apk_description.py +++ b/mozapkpublisher/test/test_update_apk_description.py @@ -15,8 +15,8 @@ def test_update_apk_description_force_locale(monkeypatch): - edit_service_mock = create_autospec(googleplay.EditService) - monkeypatch.setattr(googleplay, 'EditService', lambda _, __, ___, ____, _____: edit_service_mock) + google_play_mock = create_autospec(googleplay.WritableGooglePlay) + monkeypatch.setattr(googleplay, 'WritableGooglePlay', lambda _, __, ___: google_play_mock) monkeypatch.setattr(store_l10n, '_translations_per_google_play_locale_code', { 'google_play_locale': { 'title': 'Firefox for Android', @@ -27,17 +27,16 @@ def test_update_apk_description_force_locale(monkeypatch): }) monkeypatch.setattr(store_l10n, '_translate_moz_locate_into_google_play_one', lambda locale: 'google_play_locale') - update_apk_description('org.mozilla.firefox_beta', 'en-US', False, 'foo@developer.gserviceaccount.com', credentials, True) + update_apk_description('org.mozilla.firefox_beta', 'en-US', False, 'foo@developer.gserviceaccount.com', credentials, False) - edit_service_mock.update_listings.assert_called_once_with( + google_play_mock.update_listings.assert_called_once_with( 'google_play_locale', full_description='Long description', short_description='Short', title='Firefox for Android', ) - assert edit_service_mock.update_listings.call_count == 1 - edit_service_mock.commit_transaction.assert_called_once_with() + assert google_play_mock.update_listings.call_count == 1 def test_main(monkeypatch): diff --git a/mozapkpublisher/update_apk_description.py b/mozapkpublisher/update_apk_description.py index 193223ed..479f3d92 100755 --- a/mozapkpublisher/update_apk_description.py +++ b/mozapkpublisher/update_apk_description.py @@ -5,24 +5,23 @@ from argparse import ArgumentParser from mozapkpublisher.common import googleplay, store_l10n +from mozapkpublisher.common.googleplay import connection_for_options, WritableGooglePlay logger = logging.getLogger(__name__) def update_apk_description(package_name, force_locale, commit, service_account, google_play_credentials_file, contact_google_play): - edit_service = googleplay.EditService(service_account, google_play_credentials_file.name, package_name, commit, - contact_google_play) + connection = connection_for_options(contact_google_play, service_account, google_play_credentials_file) + with WritableGooglePlay.transaction(connection, package_name, do_not_commit=not commit) as google_play: + moz_locales = [force_locale] if force_locale else None + l10n_strings = store_l10n.get_translations_per_google_play_locale_code(package_name, moz_locales) + create_or_update_listings(google_play, l10n_strings) - moz_locales = [force_locale] if force_locale else None - l10n_strings = store_l10n.get_translations_per_google_play_locale_code(package_name, moz_locales) - create_or_update_listings(edit_service, l10n_strings) - edit_service.commit_transaction() - -def create_or_update_listings(edit_service, l10n_strings): +def create_or_update_listings(google_play, l10n_strings): for google_play_locale_code, translation in l10n_strings.items(): - edit_service.update_listings( + google_play.update_listings( google_play_locale_code, full_description=translation['long_desc'], short_description=translation['short_desc'], From 006189c48728a0847046d2208373ea6f3eb393e4 Mon Sep 17 00:00:00 2001 From: Mitchell Hentges Date: Mon, 15 Jul 2019 13:29:11 -0700 Subject: [PATCH 2/2] Moves "track" and "connection" options to the api-level --- CHANGELOG.md | 5 ++ mozapkpublisher/check_rollout.py | 11 ++- mozapkpublisher/common/googleplay.py | 73 +++++++++------ mozapkpublisher/push_apk.py | 51 +++++------ .../test/common/test_googleplay.py | 58 ++++++------ mozapkpublisher/test/test_check_rollout.py | 2 +- mozapkpublisher/test/test_push_apk.py | 89 +++++++++---------- .../test/test_update_apk_description.py | 10 ++- mozapkpublisher/update_apk_description.py | 11 ++- 9 files changed, 161 insertions(+), 149 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfcafa3d..76a8461e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Changed * Separates read-only logic from logic that requires a transaction +* `--service-account` and `--credentials` is now only required if `--do-not-contact-google-play` isn't provided +* `push_apk` and `update_apk_description` now accept types for `track` and `connection`, rather than mutually-exclusive primitive parameters + +### Fixed +* `check_rollout` no longer ignores `--do-not-contact-google-play` ## [4.1.0] - 2019-07-10 ### Removed diff --git a/mozapkpublisher/check_rollout.py b/mozapkpublisher/check_rollout.py index b54de5a0..cbfc119d 100755 --- a/mozapkpublisher/check_rollout.py +++ b/mozapkpublisher/check_rollout.py @@ -9,7 +9,7 @@ from argparse import ArgumentParser from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \ - ReadOnlyGooglePlay, GooglePlayConnection + ReadOnlyGooglePlay, connection_for_options DAY = 24 * 60 * 60 @@ -18,7 +18,7 @@ def check_rollout(google_play, days): """Check if package_name has a release on staged rollout for too long""" - track_status = google_play.get_track_status(track='production') + track_status = google_play.get_rollout_status() releases = track_status['releases'] for release in releases: if release['status'] == 'inProgress': @@ -39,11 +39,10 @@ def main(): parser.add_argument('--days', help='The time before we warn about incomplete staged rollout of a release (default: 7)', type=int, default=7) config = parser.parse_args() + connection = connection_for_options(config.contact_google_play, config.service_account, + config.google_play_credentials_file) - google_play = ReadOnlyGooglePlay.create(GooglePlayConnection.open( - config.service_account, - config.google_play_credentials_file.name - ), 'org.mozilla.firefox') + google_play = ReadOnlyGooglePlay.create(connection, 'org.mozilla.firefox') for (release, age) in check_rollout(google_play, config.days): print('fennec {} is on staged rollout at {}% but it shipped {} days ago'.format( release['name'], int(release['userFraction'] * 100), int(age / DAY))) diff --git a/mozapkpublisher/common/googleplay.py b/mozapkpublisher/common/googleplay.py index fd9337f4..fc2a1179 100644 --- a/mozapkpublisher/common/googleplay.py +++ b/mozapkpublisher/common/googleplay.py @@ -30,10 +30,33 @@ logger = logging.getLogger(__name__) +class RolloutTrack: + def __init__(self, percentage): + if not (1.0 >= percentage > 0): + raise ValueError('Rollout percentage must be (0.0, 1.0]') + self.name = 'rollout' + self.percentage = percentage + + def __eq__(self, other): + if isinstance(other, RolloutTrack): + return self.name == other.name + return False + + +class StaticTrack: + def __init__(self, name): + self.name = name + + def __eq__(self, other): + if isinstance(other, StaticTrack): + return self.name == other.name + return False + + def add_general_google_play_arguments(parser): - parser.add_argument('--service-account', help='The service account email', required=True) + parser.add_argument('--service-account', help='The service account email') parser.add_argument('--credentials', dest='google_play_credentials_file', type=argparse.FileType(mode='rb'), - help='The p12 authentication file', required=True) + help='The p12 authentication file') parser.add_argument('--commit', action='store_true', help='Commit changes onto Google Play. This action cannot be reverted.') @@ -98,15 +121,21 @@ def get_edit_resource(): update_mock.update = lambda *args, **kwargs: _ExecuteDummy('fake-update-response') edit_service_mock.tracks = lambda *args, **kwargs: update_mock edit_service_mock.listings = lambda *args, **kwargs: update_mock - edit_service_mock.apklistings = lambda *args, **kwargs: update_mock return edit_service_mock def connection_for_options(contact_google_play, service_account, credentials_file): if contact_google_play: + if service_account is None or credentials_file is None: + raise WrongArgumentGiven("Either provide '--service-account' and '--credentials', or avoid communication " + "with the real Google Play with '--do-not-contact-google-play'") return GooglePlayConnection.open(service_account, credentials_file.name) else: + if service_account is not None or credentials_file is not None: + raise WrongArgumentGiven("When using '--do-not-contact-google-play', do not use '--service-account' or " + "'--credentials'") + logger.warning('Not a single request to Google Play will be made, since `contact_google_play` was set') return MockGooglePlayConnection() @@ -122,13 +151,13 @@ def __init__(self, edit_resource, edit_id, package_name): self._edit_id = edit_id self._package_name = package_name - def get_track_status(self, track): + def get_rollout_status(self): response = self._edit_resource.tracks().get( editId=self._edit_id, - track=track, + track='production', packageName=self._package_name ).execute() - logger.debug('Track "{}" has status: {}'.format(track, response)) + logger.debug('Track "production" has status: {}'.format(response)) return response @staticmethod @@ -177,25 +206,21 @@ def upload_apk(self, apk_path): return raise - def update_track(self, track, version_codes, rollout_percentage=None): + def update_track(self, track, version_codes): body = { u'releases': [{ u'status': 'completed', u'versionCodes': version_codes, }], } - if rollout_percentage is not None: - if rollout_percentage < 0 or rollout_percentage > 100: - raise WrongArgumentGiven( - 'rollout percentage must be between 0 and 100. Value given: {}'.format( - rollout_percentage)) - body[u'userFraction'] = rollout_percentage / 100.0 # Ensure float in Python 2 + if isinstance(track, RolloutTrack): + body[u'userFraction'] = track.percentage response = self._edit_resource.tracks().update( - editId=self._edit_id, track=track, packageName=self._package_name, body=body + editId=self._edit_id, track=track.name, packageName=self._package_name, body=body ).execute() - logger.info('Track "{}" updated with: {}'.format(track, body)) + logger.info('Track "{}" updated with: {}'.format(track.name, body)) logger.debug('Track update response: {}'.format(response)) def update_listings(self, language, title, full_description, short_description): @@ -210,26 +235,16 @@ def update_listings(self, language, title, full_description, short_description): logger.info(u'Listing for language "{}" has been updated with: {}'.format(language, body)) logger.debug(u'Listing response: {}'.format(response)) - def update_whats_new(self, language, apk_version_code, whats_new): - response = self._edit_resource.apklistings().update( - editId=self._edit_id, packageName=self._package_name, language=language, - apkVersionCode=apk_version_code, body={'recentChanges': whats_new} - ).execute() - logger.info(u'What\'s new listing for ("{}", "{}") has been updated to: "{}"'.format( - language, apk_version_code, whats_new - )) - logger.debug(u'Apk listing response: {}'.format(response)) - @staticmethod @contextmanager - def transaction(connection, package_name, do_not_commit=False): + def transaction(connection, package_name, commit): edit_resource = connection.get_edit_resource() edit_id = edit_resource.insert(body={}, packageName=package_name).execute()['id'] google_play = WritableGooglePlay(edit_resource, edit_id, package_name) yield google_play - if do_not_commit: - logger.warning('Transaction not committed, since `do_not_commit` was set') - else: + if commit: edit_resource.commit(editId=edit_id, packageName=package_name) logger.info('Changes committed') logger.debug('edit_id "{}" for "{}" has been committed'.format(edit_id, package_name)) + else: + logger.warning('Transaction not committed, since `do_not_commit` was set') diff --git a/mozapkpublisher/push_apk.py b/mozapkpublisher/push_apk.py index 05c8698d..3c8765e5 100755 --- a/mozapkpublisher/push_apk.py +++ b/mozapkpublisher/push_apk.py @@ -6,20 +6,17 @@ from mozapkpublisher.common import googleplay, main_logging from mozapkpublisher.common.apk import add_apk_checks_arguments, extract_and_check_apks_metadata from mozapkpublisher.common.exceptions import WrongArgumentGiven -from mozapkpublisher.common.googleplay import WritableGooglePlay, connection_for_options +from mozapkpublisher.common.googleplay import WritableGooglePlay, connection_for_options, RolloutTrack, StaticTrack logger = logging.getLogger(__name__) def push_apk( apks, - service_account, - google_play_credentials_file, + connection, track, expected_package_names, - rollout_percentage=None, commit=True, - contact_google_play=True, skip_check_ordered_version_codes=False, skip_check_multiple_locales=False, skip_check_same_locales=False, @@ -29,16 +26,10 @@ def push_apk( Args: apks: list of APK files - service_account: Google Play service account - google_play_credentials_file: Credentials file to authenticate to Google Play - track (str): Google Play track to deploy to (e.g.: "nightly"). If "rollout" is chosen, the parameter - `rollout_percentage` must be specified as well + connection (typing.Union[GooglePlayConnection, MockGooglePlayConnection]): connection to Google Play + track (typing.Union[StaticTrack, RolloutTrack]): Google Play track to deploy to (e.g.: StaticTrack("nightly")). expected_package_names (list of str): defines what the expected package name must be. - rollout_percentage (int): percentage of users to roll out this update to. Must be a number between [0-100]. - This option is only valid if `track` is set to "rollout" commit (bool): `False` to do a dry-run - contact_google_play (bool): `False` to avoid communicating with Google Play. Useful if you're using mock - credentials. skip_checks_fennec (bool): skip Fennec-specific checks skip_check_same_locales (bool): skip check to ensure all APKs have the same locales skip_check_multiple_locales (bool): skip check to ensure all APKs have more than one locale @@ -49,11 +40,6 @@ def push_apk( # We want to tune down some logs, even when push_apk() isn't called from the command line main_logging.init() - if track == 'rollout' and rollout_percentage is None: - raise WrongArgumentGiven("When using track='rollout', rollout percentage must be provided too") - if rollout_percentage is not None and track != 'rollout': - raise WrongArgumentGiven("When using rollout-percentage, track must be set to rollout") - apks_metadata_per_paths = extract_and_check_apks_metadata( apks, expected_package_names, @@ -63,22 +49,16 @@ def push_apk( skip_check_ordered_version_codes, ) - # TODO make programmatic usage of this library provide a "GooglePlayConnection" object, rather - # than having to provide redundant information like "service_account" and "credentials" when - # "contact_google_play" is false - connection = connection_for_options(contact_google_play, service_account, google_play_credentials_file) - # Each distinct product must be uploaded in different Google Play transaction, so we split them # by package name here. split_apk_metadata = _split_apk_metadata_per_package_name(apks_metadata_per_paths) for (package_name, apks_metadata) in split_apk_metadata.items(): - with WritableGooglePlay.transaction(connection, package_name, - do_not_commit=not commit) as google_play: + with WritableGooglePlay.transaction(connection, package_name, commit) as google_play: for path, metadata in apks_metadata_per_paths.items(): google_play.upload_apk(path) all_version_codes = _get_ordered_version_codes(apks_metadata_per_paths) - google_play.update_track(track, all_version_codes, rollout_percentage) + google_play.update_track(track, all_version_codes) def _split_apk_metadata_per_package_name(apks_metadata_per_paths): @@ -119,16 +99,25 @@ def main(): config = parser.parse_args() + if config.track == 'rollout': + if config.rollout_percentage is None: + raise WrongArgumentGiven("When using '--track rollout', '--rollout-percentage' must be provided too") + track = RolloutTrack(config.rollout_percentage / 100.0) + else: + if config.rollout_percentage is not None: + raise WrongArgumentGiven("When using '--rollout-percentage', you must have '--track rollout'") + track = StaticTrack(config.track) + + connection = connection_for_options(config.contact_google_play, config.service_account, + config.google_play_credentials_file) + try: push_apk( config.apks, - config.service_account, - config.google_play_credentials_file, - config.track, + connection, + track, config.expected_package_names, - config.rollout_percentage, config.commit, - config.contact_google_play, config.skip_check_ordered_version_codes, config.skip_check_multiple_locales, config.skip_check_same_locales, diff --git a/mozapkpublisher/test/common/test_googleplay.py b/mozapkpublisher/test/common/test_googleplay.py index e99509be..f4a129fb 100644 --- a/mozapkpublisher/test/common/test_googleplay.py +++ b/mozapkpublisher/test/common/test_googleplay.py @@ -13,7 +13,7 @@ from mozapkpublisher.common.exceptions import WrongArgumentGiven from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \ WritableGooglePlay, MockGooglePlayConnection, ReadOnlyGooglePlay, GooglePlayConnection, \ - connection_for_options + connection_for_options, StaticTrack, RolloutTrack from mozapkpublisher.test import does_not_raise @@ -30,10 +30,29 @@ def test_add_general_google_play_arguments(): assert config.service_account == 'dummy@dummy' +@pytest.mark.parametrize('contact_google_play,service_account,credentials_file', ( + (False, 'account', None), + (False, None, 'file'), + (False, 'account', 'file'), + (True, None, None), + (True, 'account', None), + (True, None, 'file'), +)) +def test_connection_for_options_invalid_options(contact_google_play, service_account, credentials_file): + with pytest.raises(WrongArgumentGiven): + connection_for_options(contact_google_play, service_account, credentials_file) + + @patch.object(googleplay, 'MockGooglePlayConnection') -def test_connection_for_options_contact(mock): - connection_for_options(False, '', MagicMock) - mock.assert_called_with() +def test_connection_for_options_do_not_contact(mock): + connection_for_options(False, None, None) + mock.assert_called() + + +@patch.object(googleplay.GooglePlayConnection, 'open') +def test_connection_for_options_do_contact(mock): + connection_for_options(True, '', MagicMock()) + mock.assert_called() @patch.object(googleplay.ServiceAccountCredentials, 'from_p12_keyfile') @@ -68,7 +87,7 @@ def test_writable_google_play_commit_transaction(): connection = MockGooglePlayConnection() mock_edits_resource = MagicMock() connection.get_edit_resource = lambda: mock_edits_resource - with WritableGooglePlay.transaction(connection, 'package.name') as _: + with WritableGooglePlay.transaction(connection, 'package.name', True) as _: pass mock_edits_resource.commit.assert_called_with(editId=ANY, packageName='package.name') @@ -78,7 +97,7 @@ def test_writable_google_play_argument_do_not_commit_transaction(): connection = MockGooglePlayConnection() mock_edits_resource = MagicMock() connection.get_edit_resource = lambda: mock_edits_resource - with WritableGooglePlay.transaction(connection, 'package.name', do_not_commit=True) as _: + with WritableGooglePlay.transaction(connection, 'package.name', False) as _: pass mock_edits_resource.commit.assert_not_called() @@ -109,7 +128,7 @@ def test_get_track_status(edit_resource_mock): edit_resource_mock.tracks().get.reset_mock() google_play = ReadOnlyGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - assert google_play.get_track_status(track='production') == release_data + assert google_play.get_rollout_status() == release_data edit_resource_mock.tracks().get.assert_called_once_with( editId=1, track='production', @@ -178,7 +197,7 @@ def test_upload_apk_does_not_error_out_when_apk_is_already_published(edit_resour def test_update_track(edit_resource_mock): google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - google_play.update_track('alpha', ['2015012345', '2015012347']) + google_play.update_track(StaticTrack('alpha'), ['2015012345', '2015012347']) edit_resource_mock.tracks().update.assert_called_once_with( editId=google_play._edit_id, packageName='dummy_package_name', @@ -192,7 +211,7 @@ def test_update_track(edit_resource_mock): ) edit_resource_mock.tracks().update.reset_mock() - google_play.update_track('rollout', ['2015012345', '2015012347'], rollout_percentage=1) + google_play.update_track(RolloutTrack(0.01), ['2015012345', '2015012347']) edit_resource_mock.tracks().update.assert_called_once_with( editId=google_play._edit_id, packageName='dummy_package_name', @@ -207,12 +226,10 @@ def test_update_track(edit_resource_mock): ) -@pytest.mark.parametrize('invalid_percentage', (-1, 101)) +@pytest.mark.parametrize('invalid_percentage', (-0.01, 0.0, 1.01)) def test_update_track_should_refuse_wrong_percentage(edit_resource_mock, invalid_percentage): - google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - - with pytest.raises(WrongArgumentGiven): - google_play.update_track('rollout', ['2015012345', '2015012347'], invalid_percentage) + with pytest.raises(ValueError): + RolloutTrack(invalid_percentage) def test_update_listings(edit_resource_mock): @@ -234,16 +251,3 @@ def test_update_listings(edit_resource_mock): 'shortDescription': 'Short', } ) - - -def test_update_whats_new(edit_resource_mock): - google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name') - - google_play.update_whats_new('en-GB', '2015012345', 'Check out this cool feature!') - edit_resource_mock.apklistings().update.assert_called_once_with( - editId=google_play._edit_id, - packageName='dummy_package_name', - language='en-GB', - apkVersionCode='2015012345', - body={'recentChanges': 'Check out this cool feature!'} - ) diff --git a/mozapkpublisher/test/test_check_rollout.py b/mozapkpublisher/test/test_check_rollout.py index 5d16ebb3..07f28e26 100644 --- a/mozapkpublisher/test/test_check_rollout.py +++ b/mozapkpublisher/test/test_check_rollout.py @@ -22,7 +22,7 @@ def set_up_mocks(_requests_mock, tracks): status_code=404) google_play_mock = create_autospec(googleplay.ReadOnlyGooglePlay) - google_play_mock.get_track_status.return_value = tracks + google_play_mock.get_rollout_status.return_value = tracks return google_play_mock diff --git a/mozapkpublisher/test/test_push_apk.py b/mozapkpublisher/test/test_push_apk.py index 78212ca9..d7f73e1c 100644 --- a/mozapkpublisher/test/test_push_apk.py +++ b/mozapkpublisher/test/test_push_apk.py @@ -1,5 +1,7 @@ from contextlib import contextmanager +from mozapkpublisher.common.exceptions import WrongArgumentGiven + import mozapkpublisher import os import pytest @@ -10,7 +12,7 @@ from tempfile import NamedTemporaryFile from mozapkpublisher.common import googleplay -from mozapkpublisher.common.exceptions import WrongArgumentGiven +from mozapkpublisher.common.googleplay import StaticTrack, RolloutTrack, MockGooglePlayConnection from mozapkpublisher.push_apk import ( push_apk, main, @@ -20,12 +22,9 @@ from unittest.mock import patch -credentials = NamedTemporaryFile() apk_x86 = NamedTemporaryFile() apk_arm = NamedTemporaryFile() - APKS = [apk_x86, apk_arm] -SERVICE_ACCOUNT = 'foo@developer.gserviceaccount.com' @pytest.fixture @@ -82,26 +81,6 @@ def fake_transaction(_, __, do_not_commit): monkeypatch_.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', _metadata) -def test_invalid_rollout_percentage(): - with pytest.raises(WrongArgumentGiven): - # missing percentage - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'rollout', []) - - valid_percentage = 1 - invalid_track = 'production' - with pytest.raises(WrongArgumentGiven): - push_apk(APKS, SERVICE_ACCOUNT, credentials, invalid_track, [], rollout_percentage=valid_percentage) - - -def test_valid_rollout_percentage(writable_google_play_mock, monkeypatch): - set_up_mocks(monkeypatch, writable_google_play_mock) - valid_percentage = 50 - - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'rollout', [], rollout_percentage=valid_percentage, contact_google_play=False) - writable_google_play_mock.update_track.assert_called_once_with('rollout', ['0', '1'], valid_percentage) - writable_google_play_mock.update_track.reset_mock() - - def test_get_ordered_version_codes(): assert _get_ordered_version_codes({ 'x86': { @@ -116,12 +95,12 @@ def test_get_ordered_version_codes(): def test_upload_apk(writable_google_play_mock, monkeypatch): set_up_mocks(monkeypatch, writable_google_play_mock) - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'alpha', [], contact_google_play=False) + push_apk(APKS, MockGooglePlayConnection(), StaticTrack('alpha'), []) for apk_file in (apk_arm, apk_x86): writable_google_play_mock.upload_apk.assert_any_call(apk_file.name) - writable_google_play_mock.update_track.assert_called_once_with('alpha', ['0', '1'], None) + writable_google_play_mock.update_track.assert_called_once_with(StaticTrack('alpha'), ['0', '1']) def test_get_distinct_package_name_apk_metadata(): @@ -168,7 +147,7 @@ def test_push_apk_tunes_down_logs(monkeypatch): monkeypatch.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', MagicMock()) monkeypatch.setattr('mozapkpublisher.push_apk._split_apk_metadata_per_package_name', MagicMock()) - push_apk(APKS, SERVICE_ACCOUNT, credentials, 'alpha', [], contact_google_play=False) + push_apk(APKS, MockGooglePlayConnection(), StaticTrack('alpha'), []) main_logging_mock.init.assert_called_once_with() @@ -180,31 +159,49 @@ def test_main_bad_arguments_status_code(monkeypatch): assert exception.value.code == 2 -def test_main(monkeypatch): - incomplete_args = [ - '--package-name', 'org.mozilla.fennec_aurora', '--track', 'alpha', - '--service-account', 'foo@developer.gserviceaccount.com', - ] - - monkeypatch.setattr(sys, 'argv', incomplete_args) +@pytest.mark.parametrize('flags', ( + (['--track', 'rollout']), + (['--track', 'production', '--rollout-percentage', '50']), + (['--track', 'nightly', '--rollout-percentage', '1']), +)) +def test_parse_invalid_track(monkeypatch, flags): + file = os.path.join(os.path.dirname(__file__), 'data', 'blob') + args = [ + 'script', + '--expected-package-name', 'org.mozilla.fennec_aurora', '--do-not-contact-google-play' + ] + flags + [file] + monkeypatch.setattr(sys, 'argv', args) - with pytest.raises(SystemExit): + with pytest.raises(WrongArgumentGiven): main() + +@pytest.mark.parametrize('flags,expected_track', ( + (['--track', 'rollout', '--rollout-percentage', '50'], RolloutTrack(0.50)), + (['--track', 'production'], StaticTrack('production')), + (['--track', 'nightly'], StaticTrack('nightly')), +)) +def test_parse_valid_track(monkeypatch, flags, expected_track): file = os.path.join(os.path.dirname(__file__), 'data', 'blob') - fail_manual_validation_args = [ + args = [ 'script', - '--track', 'rollout', + '--expected-package-name', 'org.mozilla.fennec_aurora', '--do-not-contact-google-play' + ] + flags + [file] + monkeypatch.setattr(sys, 'argv', args) + + with patch.object(mozapkpublisher.push_apk, 'push_apk') as mock_push_apk: + main() + mock_push_apk.assert_called_once() + assert mock_push_apk.call_args[0][2] == expected_track + + +def test_main(monkeypatch): + incomplete_args = [ + 'script', '--expected-package-name', 'org.mozilla.fennec_aurora', '--track', 'alpha', '--service-account', 'foo@developer.gserviceaccount.com', - '--credentials', file, - '--expected-package-name', 'org.mozilla.fennec_aurora', - file ] - with patch.object(mozapkpublisher.push_apk, 'push_apk', wraps=mozapkpublisher.push_apk.push_apk) as mock_push_apk: - monkeypatch.setattr(sys, 'argv', fail_manual_validation_args) - - with pytest.raises(SystemExit): - main() + monkeypatch.setattr(sys, 'argv', incomplete_args) - assert mock_push_apk.called + with pytest.raises(SystemExit): + main() diff --git a/mozapkpublisher/test/test_update_apk_description.py b/mozapkpublisher/test/test_update_apk_description.py index d4516617..cf2722eb 100644 --- a/mozapkpublisher/test/test_update_apk_description.py +++ b/mozapkpublisher/test/test_update_apk_description.py @@ -8,6 +8,8 @@ from tempfile import NamedTemporaryFile from mozapkpublisher.common import googleplay, store_l10n +from mozapkpublisher.common.exceptions import WrongArgumentGiven +from mozapkpublisher.common.googleplay import MockGooglePlayConnection from mozapkpublisher.update_apk_description import main, update_apk_description @@ -27,7 +29,7 @@ def test_update_apk_description_force_locale(monkeypatch): }) monkeypatch.setattr(store_l10n, '_translate_moz_locate_into_google_play_one', lambda locale: 'google_play_locale') - update_apk_description('org.mozilla.firefox_beta', 'en-US', False, 'foo@developer.gserviceaccount.com', credentials, False) + update_apk_description(MockGooglePlayConnection(), 'org.mozilla.firefox_beta', 'en-US', False) google_play_mock.update_listings.assert_called_once_with( 'google_play_locale', @@ -41,13 +43,14 @@ def test_update_apk_description_force_locale(monkeypatch): def test_main(monkeypatch): incomplete_args = [ + 'script', '--package-name', 'org.mozilla.firefox_beta', '--service-account', 'foo@developer.gserviceaccount.com', ] monkeypatch.setattr(sys, 'argv', incomplete_args) - with pytest.raises(SystemExit): + with pytest.raises(WrongArgumentGiven): main() complete_args = [ @@ -57,5 +60,6 @@ def test_main(monkeypatch): '--credentials', os.path.join(os.path.dirname(__file__), 'data', 'blob') ] monkeypatch.setattr(sys, 'argv', complete_args) - monkeypatch.setattr(mozapkpublisher.update_apk_description, 'update_apk_description', lambda _, __, ___, ____, _____, ______: None) + monkeypatch.setattr(mozapkpublisher.update_apk_description, 'update_apk_description', lambda _, __, ___, ____: None) + monkeypatch.setattr(mozapkpublisher.update_apk_description, 'connection_for_options', lambda _, __, ___: None) main() diff --git a/mozapkpublisher/update_apk_description.py b/mozapkpublisher/update_apk_description.py index 479f3d92..bb385331 100755 --- a/mozapkpublisher/update_apk_description.py +++ b/mozapkpublisher/update_apk_description.py @@ -10,10 +10,8 @@ logger = logging.getLogger(__name__) -def update_apk_description(package_name, force_locale, commit, service_account, google_play_credentials_file, - contact_google_play): - connection = connection_for_options(contact_google_play, service_account, google_play_credentials_file) - with WritableGooglePlay.transaction(connection, package_name, do_not_commit=not commit) as google_play: +def update_apk_description(connection, package_name, force_locale, commit): + with WritableGooglePlay.transaction(connection, package_name, commit) as google_play: moz_locales = [force_locale] if force_locale else None l10n_strings = store_l10n.get_translations_per_google_play_locale_code(package_name, moz_locales) create_or_update_listings(google_play, l10n_strings) @@ -48,8 +46,9 @@ def main(): help='The Google play name of the app', required=True) parser.add_argument('--force-locale', help='Force to a specific locale (instead of all)') config = parser.parse_args() - update_apk_description(config.package_name, config.force_locale, config.commit, config.service_account, - config.google_play_credentials_file, config.contact_google_play) + connection = connection_for_options(config.contact_google_play, config.service_account, + config.google_play_credentials_file) + update_apk_description(connection, config.package_name, config.force_locale, config.commit) __name__ == '__main__' and main()