[deliver] Implements `verify` with `altool` for Xcode 14 validation #20738

polpielladev · 2022-10-11T21:55:57Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

As part of our CI, we use deliver's --verify_only flag (implemented by myself a few months back) to validate that our binary is fit to be uploaded to the App Store every night.

Description

Since the removal of the Transporter app from the Xcode 14.0 toolchain, this has stopped working for us. I have seen that some changes have already been implemented by @freddi-kit and released to migrate some of deliver and pilot's functionalities to using altool.

This PR builds on top of those changes and adds missing functionality to make the --verify_only flag work with altool by using its --validate-app option.

Testing Steps

Archive an app and export an IPA with an invalid (for upload) version of Xcode. An example of this would be Xcode 14.1 beta 2.
Run deliver with the verify_only flag: bundle exec fastlane deliver --ipa your_awesome_app.ipa --verify_only --verbose
Validation should fail with the following error

@joshdholtz @freddi-kit Feel free to give me a shout if you need extra context or if there is something you'd like me to change 👍

polpielladev · 2022-11-19T18:47:45Z

Hey @giginet 👋

Did you get a chance to look over this PR? I can see that you assigned yourself to it and was wondering if you had any updates. This flag is something we use at work on a nightly workflow and has quite a lot of value for us, so I'm happy to pick up any improvements/fixes that need to be done to get the flag working again with altool.

iwc · 2023-01-10T18:31:10Z

Yes, this breaks a workflow for my team as well. This would be great to have.

polpielladev · 2023-01-11T09:34:25Z

@giginet Did you get a chance to take a look at this PR? Happy to go through any changes that need making or give some more context on what the changes are doing 👍

giginet

@pol-piella

Hi! I apologize to keep waiting for you for such a long time.

I noticed altool_compatible_command might be removed in this implementation.
What do you think?
https://github.com/fastlane/fastlane/pull/20738/files#r1070544411

If you can fix this, could you try fixing it?

giginet · 2023-01-15T09:10:51Z

fastlane_core/lib/fastlane_core/itunes_transporter.rb

@@ -47,7 +47,7 @@ def build_upload_command(username, password, source = "/tmp", provider_short_nam
      not_implemented(__method__)
    end

-    def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
+    def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil, platform = nil, api_key = nil)


[nits] I think it's better to use **kwargs in the method signatures of the abstract method.

jwt is only needed for ITMSTransporter, and platform and api_key are only needed for altool.
So it's not better to have all parameters on the super method for code readability.

But this point is not important.

That's a very good point, I have just modified the abstract method to use **kwargs 👍

fastlane_core/lib/fastlane_core/itunes_transporter.rb

giginet · 2023-01-15T09:25:16Z

fastlane_core/lib/fastlane_core/itunes_transporter.rb

+      use_api_key = !@api_key.nil?
+      api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil
+
+      api_key = nil


It's unnecessary

Suggested change

api_key = nil

Removed 👍

giginet · 2023-01-15T09:40:06Z

deliver/lib/deliver/runner.rb

@@ -266,7 +266,7 @@ def submit_for_review
    # If itc_provider was explicitly specified, use it.
    # If there are multiple teams, infer the provider from the selected team name.
    # If there are fewer than two teams, don't infer the provider.
-    def transporter_for_selected_team(upload: false)
+    def transporter_for_selected_team(altool_compatible_command: false)


What's the situation altool_compatible_command to be false?

I think this flag is not necessary for ItunesTransporter.

In the previous implementation, verify task doesn't use altool, so some flags are needed to decide which executors are used.
However, in the current implementation, the transporter always uses altool when meeting the situations.
So I think this is not needed. What do you think?

I think it will still be needed as not all commands have altool implementations. The reason why the upload property existed was so that even if the conditions were met only upload would use itmstransporter, as it was implemented.

As we go ahead and add more commands, we'll need to mark them as altool_compatible so that we can slowly move them across. I might be missing something but I think it's the only way we can do it at the moment without breaking anything. Let me know what you think 👍

I meant all callers of this method seem to pass true for altool_compatible_command

https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR172
https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR209

So we can get rid of this argument. We can always pass true on L284 to reduce unnecessary states.

polpielladev · 2023-01-17T09:19:28Z

Thanks so much for reviewing @giginet, I will address the comments this evening 👍

polpielladev · 2023-01-17T09:21:50Z

@giginet Also, I have another very small PR to add support for a xcodebuild flag that would help us get some metrics at work, would you mind reviewing it if you get a chance?

#20896

Not sure what the process is to get someone to look at a PR, is there maybe a slack channel I can send a message on?

polpielladev · 2023-03-23T14:57:11Z

@giginet I think I have addressed all comments now, sorry it took a long time to do so, been a busy few weeks 😅

Let me know what you think about them and if you have any further comments/suggestions, happy to push further updates 👍

giginet · 2023-03-24T07:07:02Z

Thank you for your effort. I'll review it this weekend. 👍

giginet

It looks mostly good 👍
I appreciate your contribution and improving code quality.

As I commented, I believe in removing the argument of transporter_for_selected_team.

If you can fix the comments, I'll merge this soon.

diff --git a/deliver/lib/deliver/runner.rb b/deliver/lib/deliver/runner.rb
index a2b433efe..2494af5a3 100644
--- a/deliver/lib/deliver/runner.rb
+++ b/deliver/lib/deliver/runner.rb
@@ -169,7 +169,7 @@ module Deliver
       pkg_path = options[:pkg]
 
       platform = options[:platform]
-      transporter = transporter_for_selected_team(altool_compatible_command: true)
+      transporter = transporter_for_selected_team
 
       case platform
       when "ios", "appletvos"
@@ -206,7 +206,7 @@ module Deliver
       pkg_path = options[:pkg]
 
       platform = options[:platform]
-      transporter = transporter_for_selected_team(altool_compatible_command: true)
+      transporter = transporter_for_selected_team
 
       case platform
       when "ios", "appletvos"
@@ -266,7 +266,7 @@ module Deliver
     # If itc_provider was explicitly specified, use it.
     # If there are multiple teams, infer the provider from the selected team name.
     # If there are fewer than two teams, don't infer the provider.
-    def transporter_for_selected_team(altool_compatible_command: false)
+    def transporter_for_selected_team
       # Use JWT auth
       api_token = Spaceship::ConnectAPI.token
       api_key = if options[:api_key].nil? && !api_token.nil?
@@ -281,12 +281,12 @@ module Deliver
 
       unless api_token.nil?
         api_token.refresh! if api_token.expired?
-        return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, altool_compatible_command: altool_compatible_command, api_key: api_key)
+        return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, altool_compatible_command: true, api_key: api_key)
       end
 
       tunes_client = Spaceship::ConnectAPI.client.tunes_client
 
-      generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], altool_compatible_command: altool_compatible_command, api_key: api_key)
+      generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], altool_compatible_command: true, api_key: api_key)
       return generic_transporter unless options[:itc_provider].nil? && tunes_client.teams.count > 1
 
       begin
@@ -294,7 +294,7 @@ module Deliver
         name = team['name']
         provider_id = generic_transporter.provider_ids[name]
         UI.verbose("Inferred provider id #{provider_id} for team #{name}.")
-        return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, altool_compatible_command: altool_compatible_command, api_key: api_key)
+        return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, altool_compatible_command: true, api_key: api_key)
       rescue => ex
         UI.verbose("Couldn't infer a provider short name for team with id #{tunes_client.team_id} automatically: #{ex}. Proceeding without provider short name.")
         return generic_transporter

giginet · 2023-03-26T07:19:59Z

deliver/lib/deliver/runner.rb

@@ -266,7 +266,7 @@ def submit_for_review
    # If itc_provider was explicitly specified, use it.
    # If there are multiple teams, infer the provider from the selected team name.
    # If there are fewer than two teams, don't infer the provider.
-    def transporter_for_selected_team(upload: false)
+    def transporter_for_selected_team(altool_compatible_command: false)


I meant all callers of this method seem to pass true for altool_compatible_command

https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR172
https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR209

So we can get rid of this argument. We can always pass true on L284 to reduce unnecessary states.

giginet · 2023-03-26T07:21:10Z

deliver/lib/deliver/runner.rb

@@ -281,20 +281,20 @@ def transporter_for_selected_team(upload: false)

      unless api_token.nil?
        api_token.refresh! if api_token.expired?
-        return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, upload: upload, api_key: api_key)
+        return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, altool_compatible_command: altool_compatible_command, api_key: api_key)


We can always pass altool_compatible_command: true because of the above comment.

fastlane_core/lib/fastlane_core/itunes_transporter.rb

giginet · 2023-03-26T07:38:20Z

fastlane_core/lib/fastlane_core/itunes_transporter.rb

-      UI.verbose(@transporter_executor.build_verify_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt_placeholder))
+      # Handle AppStore Connect API
+      use_api_key = !@api_key.nil?
+      api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil


It's better to be commented for code readability 👍

Suggested change

api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil

# Masking credentials for verbose outputs

api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil

polpielladev · 2023-03-26T12:43:41Z

Hi @giginet 👋 Thank you so much for the help and the awesome feedback. I have addressed the comments you suggested now 👍

giginet · 2023-03-26T14:28:50Z

fastlane_core/lib/fastlane_core/itunes_transporter.rb

+                     tmp_asset_path = Dir.mktmpdir
+                     FileUtils.cp(asset_path, tmp_asset_path)
+                     tmp_asset_path


I'm so sorry, my suggestion was completely wrong.

Filenames are not randomized. and tmp_asset_path indicates the parent directory path...

It have to revert to your implementation 🙏

Suggested change

tmp_asset_path = Dir.mktmpdir

FileUtils.cp(asset_path, tmp_asset_path)

tmp_asset_path

new_file_name = "#{SecureRandom.uuid}#{File.extname(asset_path)}"

tmp_asset_path = File.join(Dir.tmpdir, new_file_name)

FileUtils.cp(asset_path, tmp_asset_path)

tmp_asset_path

Ah no problem! I have reverted the changes to the original implementation now 👍

giginet

LGTM 🚀
I'm sorry to wait for you to start reviewing for a long time.
I'll merge this after checking.

Thanks a lot again! 😄

polpielladev · 2023-03-27T08:40:31Z

LGTM 🚀 I'm sorry to wait for you to start reviewing for a long time. I'll merge this after checking.

Thanks a lot again! 😄

No problem at all honestly! Thanks so much for all your help on this, I really appreciate it 🎉

giginet · 2023-03-28T14:24:45Z

I tried this command for the latest Xcode, it works!

deliver(
  verify_only: true
)

polpielladev added 7 commits October 11, 2022 20:43

Adds support for build_verify_command

73d0982

Updates naming of variable to show the inclusion of verify command

b429756

Adds required platform option

8fd629f

Allows passing in an asset_path to upload values correctly

f6bc50a

Updates unit tests for deliver component

af7f6f9

Fixes failing tests in itunes_transporter_spec

7aa1045

Runs rubocop -a

c8b568b

giginet self-requested a review October 15, 2022 09:59

polpielladev mentioned this pull request Oct 22, 2022

[deliver] App Store Connect Validation Fails with Xcode 14 ("Could not find transporter") #20668

Open

4 tasks

fastlane-bot added the status: needs-attention label Oct 26, 2022

Merge branch 'fastlane:master' into master

e609699

polpielladev force-pushed the master branch from 57becc2 to e609699 Compare November 30, 2022 17:43

polpielladev changed the title ~~Implements verify with altool for Xcode 14 validation~~ [deliver] Implements verify with altool for Xcode 14 validation Nov 30, 2022

giginet reviewed Jan 15, 2023

View reviewed changes

giginet self-assigned this Jan 15, 2023

giginet added the tool: deliver label Jan 15, 2023

polpielladev and others added 3 commits March 23, 2023 12:38

Merge branch 'fastlane:master' into master

cf14c9e

Uses **kwargs for abstract build_verify_command method

7af747f

Fixes rubocop violation

e046af1

giginet self-requested a review March 26, 2023 06:31

giginet reviewed Mar 26, 2023

View reviewed changes

polpielladev added 2 commits March 26, 2023 13:23

Removes altool_compatible_command argument from deliver runner

d761225

Fixes further PR comments

c1bc3e8

polpielladev requested a review from giginet March 26, 2023 12:44

giginet reviewed Mar 26, 2023

View reviewed changes

Reverts changes to temporary path code

7f1dcde

giginet approved these changes Mar 27, 2023

View reviewed changes

giginet merged commit ae9277a into fastlane:master Mar 28, 2023
1 check passed

sensuikan1973 mentioned this pull request Mar 28, 2023

upload_to_app_store with verify_only option sensuikan1973/pedax#1382

Closed

sameh0 mentioned this pull request Apr 2, 2023

[deliver] Implements verify with altool for Xcode 14 validation (… sameh0/fastlane#1

Merged

5 tasks

sensuikan1973 mentioned this pull request Apr 10, 2023

change fastlane gem ref sensuikan1973/pedax#1413

Merged

joshdholtz mentioned this pull request Apr 16, 2023

Version bump to 2.212.2 #21203

Merged

Katsz mentioned this pull request Apr 24, 2023

Deliver fails after updating to Xcode 14.3 #21208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[deliver] Implements `verify` with `altool` for Xcode 14 validation #20738

[deliver] Implements `verify` with `altool` for Xcode 14 validation #20738

polpielladev commented Oct 11, 2022 •

edited

polpielladev commented Nov 19, 2022

iwc commented Jan 10, 2023

polpielladev commented Jan 11, 2023

giginet left a comment •

edited

giginet Jan 15, 2023

polpielladev Mar 23, 2023 •

edited

giginet Jan 15, 2023

polpielladev Mar 23, 2023

giginet Jan 15, 2023

polpielladev Mar 23, 2023

giginet Mar 26, 2023

polpielladev commented Jan 17, 2023

polpielladev commented Jan 17, 2023 •

edited

polpielladev commented Mar 23, 2023

giginet commented Mar 24, 2023

giginet left a comment

giginet Mar 26, 2023

giginet Mar 26, 2023

giginet Mar 26, 2023

polpielladev commented Mar 26, 2023

giginet Mar 26, 2023

polpielladev Mar 26, 2023

giginet left a comment

polpielladev commented Mar 27, 2023

giginet commented Mar 28, 2023

	api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil
	# Masking credentials for verbose outputs
	api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil

[deliver] Implements verify with altool for Xcode 14 validation #20738

[deliver] Implements verify with altool for Xcode 14 validation #20738

Conversation

polpielladev commented Oct 11, 2022 • edited

Checklist

Motivation and Context

Description

Testing Steps

polpielladev commented Nov 19, 2022

iwc commented Jan 10, 2023

polpielladev commented Jan 11, 2023

giginet left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polpielladev Mar 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polpielladev commented Jan 17, 2023

polpielladev commented Jan 17, 2023 • edited

polpielladev commented Mar 23, 2023

giginet commented Mar 24, 2023

giginet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polpielladev commented Mar 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giginet left a comment

Choose a reason for hiding this comment

polpielladev commented Mar 27, 2023

giginet commented Mar 28, 2023

[deliver] Implements `verify` with `altool` for Xcode 14 validation #20738

[deliver] Implements `verify` with `altool` for Xcode 14 validation #20738

polpielladev commented Oct 11, 2022 •

edited

giginet left a comment •

edited

polpielladev Mar 23, 2023 •

edited

polpielladev commented Jan 17, 2023 •

edited