diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae02137..76a8461e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ 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 +* `--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 * `--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..cbfc119d 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, connection_for_options 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_rollout_status() releases = track_status['releases'] for release in releases: if release['status'] == 'inProgress': @@ -39,13 +39,11 @@ 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) - edit_service = EditService( - config.service_account, - config.google_play_credentials_file.name, - package_name='org.mozilla.firefox', - ) - for (release, age) in check_rollout(edit_service, config.days): + 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/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..fc2a1179 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,15 +25,38 @@ # 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__) +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.') @@ -41,57 +66,124 @@ 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) - self._edit_id = None + 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 + + 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() + + +class ReadOnlyGooglePlay: + """Read-only access to the Google Play store + + Create an instance by calling ReadOnlyGooglePlay.create() instead of using the constructor + """ - @transaction_required - def get_track_status(self, track): - response = self._service.tracks().get( - editId=self._edit_id, track=track, packageName=self._package_name + 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_rollout_status(self): + response = self._edit_resource.tracks().get( + editId=self._edit_id, + track='production', + packageName=self._package_name ).execute() - logger.debug(u'Track "{}" has status: {}'.format(track, response)) + logger.debug('Track "production" has status: {}'.format(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,104 +195,56 @@ 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): + 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._service.tracks().update( - editId=self._edit_id, track=track, packageName=self._package_name, body=body + response = self._edit_resource.tracks().update( + 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)) - @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( - 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)) - - -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, 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 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 7e11d067..3c8765e5 100755 --- a/mozapkpublisher/push_apk.py +++ b/mozapkpublisher/push_apk.py @@ -6,19 +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, 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, @@ -28,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 @@ -48,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, @@ -65,40 +52,13 @@ def push_apk( # 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, - ) + with WritableGooglePlay.transaction(connection, package_name, commit) as google_play: + for path, metadata in apks_metadata_per_paths.items(): + google_play.upload_apk(path) - -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) - - 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) def _split_apk_metadata_per_package_name(apks_metadata_per_paths): @@ -139,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 ec8b96f1..f4a129fb 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, StaticTrack, RolloutTrack from mozapkpublisher.test import does_not_raise @@ -25,75 +30,80 @@ 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() +@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) - 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 - _monkeypatch.setattr('mozapkpublisher.common.googleplay._connect', lambda _, __: general_service_mock) - return edit_service_mock +@patch.object(googleplay, 'MockGooglePlayConnection') +def test_connection_for_options_do_not_contact(mock): + connection_for_options(False, None, None) + mock.assert_called() -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') +@patch.object(googleplay.GooglePlayConnection, 'open') +def test_connection_for_options_do_contact(mock): + connection_for_options(True, '', MagicMock()) + mock.assert_called() -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') +@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_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() +@pytest.fixture +def edit_resource_mock(): + edit_resource = MagicMock() + new_transaction_mock = MagicMock() - assert edit_service._edit_id != old_edit_id + 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_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_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') - 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_not_called() -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_commit_transaction(): + connection = MockGooglePlayConnection() + mock_edits_resource = MagicMock() + connection.get_edit_resource = lambda: mock_edits_resource + with WritableGooglePlay.transaction(connection, 'package.name', True) as _: + pass + + mock_edits_resource.commit.assert_called_with(editId=ANY, packageName='package.name') + - 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() +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', False) as _: + pass + 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 +123,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_rollout_status() == 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 +171,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 +183,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(StaticTrack('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 +210,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(RolloutTrack(0.01), ['2015012345', '2015012347']) + edit_resource_mock.tracks().update.assert_called_once_with( + editId=google_play._edit_id, packageName='dummy_package_name', track='rollout', body={ @@ -220,27 +226,23 @@ 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') - - with pytest.raises(WrongArgumentGiven): - edit_service.update_track('rollout', ['2015012345', '2015012347'], invalid_percentage) +@pytest.mark.parametrize('invalid_percentage', (-0.01, 0.0, 1.01)) +def test_update_track_should_refuse_wrong_percentage(edit_resource_mock, invalid_percentage): + with pytest.raises(ValueError): + RolloutTrack(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={ @@ -249,17 +251,3 @@ def test_update_listings(monkeypatch): 'shortDescription': 'Short', } ) - - -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') - - 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, - 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 7dccea93..07f28e26 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_rollout_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..d7f73e1c 100644 --- a/mozapkpublisher/test/test_push_apk.py +++ b/mozapkpublisher/test/test_push_apk.py @@ -1,3 +1,7 @@ +from contextlib import contextmanager + +from mozapkpublisher.common.exceptions import WrongArgumentGiven + import mozapkpublisher import os import pytest @@ -8,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, @@ -18,20 +22,17 @@ 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 -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,28 +73,12 @@ def _metadata(*args, **kwargs): } } - monkeypatch_.setattr(googleplay, 'EditService', lambda _, __, ___, commit, contact_google_play: edit_service_mock_) - monkeypatch_.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', _metadata) - - -def test_invalid_rollout_percentage(edit_service_mock, monkeypatch): - 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) - + @contextmanager + def fake_transaction(_, __, do_not_commit): + yield writable_google_play_mock_ -def test_valid_rollout_percentage(edit_service_mock, monkeypatch): - set_up_mocks(monkeypatch, edit_service_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() + monkeypatch_.setattr(googleplay.WritableGooglePlay, 'transaction', fake_transaction) + monkeypatch_.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', _metadata) def test_get_ordered_version_codes(): @@ -107,16 +92,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, MockGooglePlayConnection(), StaticTrack('alpha'), []) 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(StaticTrack('alpha'), ['0', '1']) def test_get_distinct_package_name_apk_metadata(): @@ -162,9 +146,8 @@ 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) + push_apk(APKS, MockGooglePlayConnection(), StaticTrack('alpha'), []) main_logging_mock.init.assert_called_once_with() @@ -176,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 f86a7136..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 @@ -15,8 +17,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,28 +29,28 @@ 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(MockGooglePlayConnection(), 'org.mozilla.firefox_beta', 'en-US', 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): 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 = [ @@ -58,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 193223ed..bb385331 100755 --- a/mozapkpublisher/update_apk_description.py +++ b/mozapkpublisher/update_apk_description.py @@ -5,24 +5,21 @@ 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) +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) - 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'], @@ -49,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()