Miscellaneous cleanup. (#350)

- consistently end messages with period (except in cases where a sentence ends with a colon followed by a value
- sleep before polling for upload status (because the server needs some time for asynchronous processing after upload)
- reorder/refactor some code

Author: kaibolay

lib/fastlane/plugin/firebase_app_distribution/actions/firebase_app_distribution_action.rb

@@ -20,8 +20,8 @@ class FirebaseAppDistributionAction < Action
       extend Helper::FirebaseAppDistributionHelper
 
       DEFAULT_UPLOAD_TIMEOUT_SECONDS = 300
-      MAX_POLLING_RETRIES = 60
-      POLLING_INTERVAL_SECONDS = 5
+      UPLOAD_MAX_POLLING_RETRIES = 60
+      UPLOAD_POLLING_INTERVAL_SECONDS = 5
 
       def self.run(params)
         params.values # to validate all inputs before looking for the ipa/apk/aab
@@ -32,15 +32,16 @@ def self.run(params)
         timeout = get_upload_timeout(params)
 
         binary_path = get_binary_path(platform, params)
-        UI.user_error!("Couldn't find binary") if binary_path.nil?
-        UI.user_error!("Couldn't find binary at path #{binary_path}") unless File.exist?(binary_path)
+        UI.user_error!("Couldn't find binary.") if binary_path.nil?
+        UI.user_error!("Couldn't find binary at path #{binary_path}.") unless File.exist?(binary_path)
         binary_type = binary_type_from_path(binary_path)
 
         # TODO(lkellogg): This sets the send timeout for all POST requests made by the client, but
         # ideally the timeout should only apply to the binary upload
         init_google_api_client(params[:debug], timeout)
+        authorization = get_authorization(params[:service_credentials_file], params[:firebase_cli_token], params[:debug])
         client = Google::Apis::FirebaseappdistributionV1::FirebaseAppDistributionService.new
-        client.authorization = get_authorization(params[:service_credentials_file], params[:firebase_cli_token], params[:debug])
+        client.authorization = authorization
 
         # If binary is an AAB, get the AAB info for this app, which includes the integration state
         # and certificate data
@@ -50,9 +51,9 @@ def self.run(params)
         end
 
         binary_type = binary_type_from_path(binary_path)
-        UI.message("⌛ Uploading the #{binary_type}.")
+        UI.message("📡 Uploading the #{binary_type}.")
         operation = upload_binary(app_name, binary_path, client, timeout)
-        UI.message("🕵️ Validating upload.")
+        UI.message("🕵️ Validating upload…")
         release = poll_upload_release_operation(client, operation, binary_type)
 
         if binary_type == :AAB && aab_info && !aab_certs_included?(aab_info.test_certificate)
@@ -214,24 +215,22 @@ def self.release_notes(params)
       end
 
       def self.poll_upload_release_operation(client, operation, binary_type)
-        operation = client.get_project_app_release_operation(operation.name)
-        MAX_POLLING_RETRIES.times do
+        UPLOAD_MAX_POLLING_RETRIES.times do
+          sleep(UPLOAD_POLLING_INTERVAL_SECONDS)
+          operation = client.get_project_app_release_operation(operation.name)
           if operation.done && operation.response && operation.response['release']
             release = extract_release(operation)
-            result = operation.response['result']
-            if result == 'RELEASE_UPDATED'
+            case operation.response['result']
+            when 'RELEASE_UPDATED'
               UI.success("✅ Uploaded #{binary_type} successfully; updated provisioning profile of existing release #{release_version(release)}.")
-              break
-            elsif result == 'RELEASE_UNMODIFIED'
+            when 'RELEASE_UNMODIFIED'
               UI.success("✅ The same #{binary_type} was found in release #{release_version(release)} with no changes, skipping.")
-              break
             else
               UI.success("✅ Uploaded #{binary_type} successfully and created release #{release_version(release)}.")
             end
             break
           elsif !operation.done
-            sleep(POLLING_INTERVAL_SECONDS)
-            operation = client.get_project_app_release_operation(operation.name)
+            next
           else
             if operation.error && operation.error.message
               UI.user_error!("#{ErrorMessage.upload_binary_error(binary_type)}: #{operation.error.message}")
@@ -350,7 +349,7 @@ def self.available_options
                                        verify_block: proc do |value|
                                          UI.user_error!("firebase_app_distribution: '#{value}' is not a valid value for android_artifact_type. Should be 'APK' or 'AAB'") unless ['APK', 'AAB'].include?(value)
                                        end),
-          # Generic
+          # General
           FastlaneCore::ConfigItem.new(key: :app,
                                        env_name: "FIREBASEAPPDISTRO_APP",
                                        description: "Your app's Firebase App ID. You can find the App ID in the Firebase console, on the General Settings page",
@@ -361,6 +360,18 @@ def self.available_options
                                        env_name: "FIREBASEAPPDISTRO_FIREBASE_CLI_PATH",
                                        description: "The absolute path of the firebase cli command",
                                        type: String),
+          FastlaneCore::ConfigItem.new(key: :debug,
+                                      description: "Print verbose debug output",
+                                      optional: true,
+                                      default_value: false,
+                                      type: Boolean),
+
+          # Release Distribution
+          FastlaneCore::ConfigItem.new(key: :upload_timeout,
+                                       description: "The amount of seconds before the upload will  timeout, if not completed",
+                                       optional: true,
+                                       default_value: DEFAULT_UPLOAD_TIMEOUT_SECONDS,
+                                       type: Integer),
           FastlaneCore::ConfigItem.new(key: :groups,
                                        env_name: "FIREBASEAPPDISTRO_GROUPS",
                                        description: "The group aliases used for distribution, separated by commas",
@@ -391,24 +402,16 @@ def self.available_options
                                        description: "Release notes file for this build",
                                        optional: true,
                                        type: String),
+
+          # Auth
           FastlaneCore::ConfigItem.new(key: :firebase_cli_token,
                                        description: "Auth token generated using the Firebase CLI's login:ci command",
                                        optional: true,
                                        type: String),
-          FastlaneCore::ConfigItem.new(key: :debug,
-                                       description: "Print verbose debug output",
-                                       optional: true,
-                                       default_value: false,
-                                       is_string: false),
           FastlaneCore::ConfigItem.new(key: :service_credentials_file,
                                        description: "Path to Google service account json",
                                        optional: true,
-                                       type: String),
-          FastlaneCore::ConfigItem.new(key: :upload_timeout,
-                                       description: "The amount of seconds before the upload will timeout, if not completed",
-                                       optional: true,
-                                       default_value: DEFAULT_UPLOAD_TIMEOUT_SECONDS,
-                                       type: Integer)
+                                       type: String)
         ]
       end
 

lib/fastlane/plugin/firebase_app_distribution/helper/firebase_app_distribution_error_message.rb

@@ -1,46 +1,46 @@
 module ErrorMessage
   MISSING_CREDENTIALS = "Missing authentication credentials. Set up Application Default Credentials, your Firebase refresh token, or sign in with the Firebase CLI, and try again."
-  MISSING_APP_ID = "Missing app id. Please check that the app parameter is set and try again"
-  SERVICE_CREDENTIALS_NOT_FOUND = "Service credentials file does not exist. Please check the service credentials path and try again"
-  PARSE_SERVICE_CREDENTIALS_ERROR = "Failed to extract service account information from the service credentials file"
-  PARSE_FIREBASE_TOOLS_JSON_ERROR = "Encountered error parsing json file. Ensure the firebase-tools.json file is formatted correctly"
-  UPLOAD_RELEASE_NOTES_ERROR = "App Distribution halted because it had a problem uploading release notes"
-  UPLOAD_TESTERS_ERROR = "App Distribution halted because it had a problem adding testers/groups"
-  GET_RELEASE_TIMEOUT = "App Distribution failed to fetch release information"
+  MISSING_APP_ID = "Missing app id. Please check that the app parameter is set and try again."
+  SERVICE_CREDENTIALS_NOT_FOUND = "Service credentials file does not exist. Please check the service credentials path and try again."
+  PARSE_SERVICE_CREDENTIALS_ERROR = "Failed to extract service account information from the service credentials file."
+  PARSE_FIREBASE_TOOLS_JSON_ERROR = "Encountered error parsing json file. Ensure the firebase-tools.json file is formatted correctly."
+  UPLOAD_RELEASE_NOTES_ERROR = "App Distribution halted because it had a problem uploading release notes."
+  UPLOAD_TESTERS_ERROR = "App Distribution halted because it had a problem adding testers/groups."
+  GET_RELEASE_TIMEOUT = "App Distribution failed to fetch release information."
   REFRESH_TOKEN_ERROR = "App Distribution could not generate credentials from the refresh token specified."
-  APP_NOT_ONBOARDED_ERROR = "App Distribution not onboarded"
+  APP_NOT_ONBOARDED_ERROR = "App Distribution not onboarded."
   INVALID_APP_ID = "App Distribution could not find your app. Make sure to onboard your app by pressing the \"Get started\" button on the App Distribution page in the Firebase console: https://console.firebase.google.com/project/_/appdistribution. App ID"
   INVALID_PROJECT = "App Distribution could not find your Firebase project. Make sure to onboard an app in your project by pressing the \"Get started\" button on the App Distribution page in the Firebase console: https://console.firebase.google.com/project/_/appdistribution."
   INVALID_PATH = "Could not read content from"
   INVALID_TESTERS = "Could not enable access for testers. Check that the tester emails are formatted correctly, the groups exist and you are using group aliases (not group names) for specifying groups."
   INVALID_TESTER_GROUP = "App Distribution could not find your tester group. Make sure that it exists before trying to add testers, and that the group alias is specified correctly."
   INVALID_TESTER_GROUP_NAME = "The tester group name should be 4-63 characters, and valid characters are /[a-z][0-9]-/."
-  INVALID_RELEASE_NOTES = "Failed to set release notes"
-  SERVICE_CREDENTIALS_ERROR = "App Distribution could not generate credentials from the service credentials file specified"
+  INVALID_RELEASE_NOTES = "Failed to set release notes."
+  SERVICE_CREDENTIALS_ERROR = "App Distribution could not generate credentials from the service credentials file specified."
   PLAY_ACCOUNT_NOT_LINKED = "This project is not linked to a Google Play account."
   APP_NOT_PUBLISHED = "This app is not published in the Google Play console."
   NO_APP_WITH_GIVEN_BUNDLE_ID_IN_PLAY_ACCOUNT = "App with matching package name does not exist in Google Play."
   PLAY_IAS_TERMS_NOT_ACCEPTED = "You must accept the Play Internal App Sharing (IAS) terms to upload AABs."
   INVALID_EMAIL_ADDRESS = "You passed an invalid email address."
-  TESTER_LIMIT_VIOLATION = "Creating testers would exceed tester limit"
+  TESTER_LIMIT_VIOLATION = "Creating testers would exceed tester limit."
 
   def self.aab_upload_error(aab_state)
     "Failed to process the AAB: #{aab_state}"
   end
 
   def self.binary_not_found(binary_type)
-    "Could not find the #{binary_type}. Make sure you set the #{binary_type} path parameter to point to your #{binary_type}"
+    "Could not find the #{binary_type}. Make sure you set the #{binary_type} path parameter to point to your #{binary_type}."
   end
 
   def self.parse_binary_metadata_error(binary_type)
-    "Failed to extract #{binary_type} metadata from the #{binary_type} path"
+    "Failed to extract #{binary_type} metadata from the #{binary_type} path."
   end
 
   def self.upload_binary_error(binary_type)
-    "App Distribution halted because it had a problem uploading the #{binary_type}"
+    "App Distribution halted because it had a problem uploading the #{binary_type}."
   end
 
   def self.binary_processing_error(binary_type)
-    "App Distribution failed to process the #{binary_type}"
+    "App Distribution failed to process the #{binary_type}."
   end
 end

spec/firebase_app_distribution_action_spec.rb

@@ -228,13 +228,13 @@
     it 'raises error if it cannot find a valid binary path' do
       expect do
         action.run(params.merge(ipa_path: nil))
-      end.to raise_error("Couldn't find binary")
+      end.to raise_error("Couldn't find binary.")
     end
 
     it 'raises error if binary does not exist' do
       expect do
         action.run(params)
-      end.to raise_error("Couldn't find binary at path debug.ipa")
+      end.to raise_error("Couldn't find binary at path debug.ipa.")
     end
 
     describe 'with android app' do
@@ -319,7 +319,7 @@ def stub_get_aab_info(integration_state = 'INTEGRATED')
         end
 
         it 'crashes if it exceeds polling threshold' do
-          stub_const('Fastlane::Actions::FirebaseAppDistributionAction::MAX_POLLING_RETRIES', 0)
+          stub_const('Fastlane::Actions::FirebaseAppDistributionAction::UPLOAD_MAX_POLLING_RETRIES', 0)
           allow_any_instance_of(V1Api::FirebaseAppDistributionService)
             .to receive(:http)
             .and_return({ name: 'operation-name' }.to_json)