Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[deliver][pilot] use altool instead of using iTMSTransporter for Xcode 14 #20631

Merged
merged 69 commits into from Sep 15, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
83cdc08
Use altool instead of using iTMSTransporter for Xcode 14
freddi-kit Sep 7, 2022
ec4cfee
fix linter error
freddi-kit Sep 7, 2022
f1e0575
fix linter error
freddi-kit Sep 7, 2022
c3a66db
fix linter error
freddi-kit Sep 7, 2022
39c919e
use keyword function for transporter_for_selected_team
freddi-kit Sep 9, 2022
74f681d
fix typo and move platform string handling
freddi-kit Sep 9, 2022
6d10343
remove unecessary return
freddi-kit Sep 9, 2022
ce9b12d
create should_use_altool
freddi-kit Sep 9, 2022
40337e3
fix to use unless to make code one line
freddi-kit Sep 9, 2022
0f2a3d4
make code oneline by using unless
freddi-kit Sep 9, 2022
d675551
improve error message
freddi-kit Sep 9, 2022
fa388c7
fix to support apple connect api key
freddi-kit Sep 9, 2022
dcb97a8
fix jwt check
freddi-kit Sep 9, 2022
5bd7d3f
fix to set? for boolean function
freddi-kit Sep 9, 2022
2023aa8
fix rubocup issue
freddi-kit Sep 9, 2022
7a13c48
fix rubocup issue
freddi-kit Sep 9, 2022
8df9559
fix test
freddi-kit Sep 9, 2022
ed28c90
fix to call upload without replacement
freddi-kit Sep 9, 2022
375f951
fix typo
freddi-kit Sep 9, 2022
711eb3c
fix checking apikey available or not
freddi-kit Sep 9, 2022
747178f
fix to generate p8 file for every calling altool
freddi-kit Sep 10, 2022
13afc8c
PR review fixes (Remove unused file, autogenerated by uni-tests + imp…
crazymanish Sep 10, 2022
d36bf23
Fix itunes_transporter_spec.rb which was failing in Xcode14 https://g…
crazymanish Sep 10, 2022
e4e44ac
Fix helper_spec.rb which was failing in Xcode14 https://github.com/fa…
crazymanish Sep 10, 2022
4f677a6
support base64 key_content for using altool with appstore connect apikey
freddi-kit Sep 10, 2022
9988e33
fix typo
freddi-kit Sep 10, 2022
4ebf009
fix to remove un-necessary begin
freddi-kit Sep 10, 2022
ddb437a
fix to use unless
freddi-kit Sep 10, 2022
b4f5324
improve showing error on UI
freddi-kit Sep 10, 2022
44bf7aa
fix to remove warning in altool executer
freddi-kit Sep 10, 2022
01a5873
improve error log
freddi-kit Sep 10, 2022
7490c5c
add some small fix
freddi-kit Sep 10, 2022
388726b
fix code format
freddi-kit Sep 11, 2022
b070b58
fix to remove unnecessary return
freddi-kit Sep 11, 2022
1cdc817
fix to use File.open() |p8|
freddi-kit Sep 11, 2022
c82e99a
fix to use mktmpdir for saving p8 temp
freddi-kit Sep 12, 2022
bdbacb5
fix linter issue
freddi-kit Sep 12, 2022
00c974c
fix to use wb option when making p8 file
freddi-kit Sep 12, 2022
1994860
add unit test for altool case
freddi-kit Sep 12, 2022
16249e7
fix test format
freddi-kit Sep 12, 2022
8ab40fa
remove space
freddi-kit Sep 12, 2022
22cfcec
support api json path
freddi-kit Sep 13, 2022
3fc11fa
add some comment and fix code style
freddi-kit Sep 13, 2022
980f189
add comment about error info
freddi-kit Sep 13, 2022
669bebc
compose return expression in one line
freddi-kit Sep 13, 2022
98e1a88
fix to use checking raise_error
freddi-kit Sep 13, 2022
355cc9d
use context for matching rpec
freddi-kit Sep 13, 2022
0f6bfcb
fix code's readability
freddi-kit Sep 13, 2022
f4cd020
fix lint
freddi-kit Sep 13, 2022
95e7cb8
fix lint
freddi-kit Sep 13, 2022
38f23ee
fix lint
freddi-kit Sep 13, 2022
30839b9
fix ids provider problem
freddi-kit Sep 14, 2022
a2ea2fb
add comments
freddi-kit Sep 14, 2022
874320c
add test for provider ids
freddi-kit Sep 14, 2022
ab02d6d
fix api key condition to make code more readable
freddi-kit Sep 14, 2022
71e57d6
add more detailed comment
freddi-kit Sep 14, 2022
b518207
fix typo
freddi-kit Sep 14, 2022
8d5bb62
improve specifying substring
freddi-kit Sep 14, 2022
5beca1e
fix index
freddi-kit Sep 14, 2022
4f4d3ee
fix altool providers output to parse json
freddi-kit Sep 14, 2022
cd2c504
fix code format
freddi-kit Sep 14, 2022
4b93a81
fix hash problem if user pass key via environment variable
freddi-kit Sep 14, 2022
18f460f
remove unnecessary !
freddi-kit Sep 14, 2022
ac7358d
fix null handle
freddi-kit Sep 14, 2022
07181a7
dummy commit for test
freddi-kit Sep 14, 2022
599ab2b
dummy commit for test
freddi-kit Sep 14, 2022
8be9c7e
change the implementation place of provider id parser
freddi-kit Sep 15, 2022
8e7d6f3
make absract method
freddi-kit Sep 15, 2022
c005aae
add raise for not implemented functions
freddi-kit Sep 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions deliver/lib/deliver/runner.rb
Expand Up @@ -206,7 +206,7 @@ def upload_binary
pkg_path = options[:pkg]

platform = options[:platform]
transporter = transporter_for_selected_team
transporter = transporter_for_selected_team(upload: true)

case platform
when "ios", "appletvos"
Expand All @@ -216,15 +216,15 @@ def upload_binary
package_path: "/tmp",
platform: platform
)
result = transporter.upload(package_path: package_path, asset_path: ipa_path)
result = transporter.upload(package_path: package_path, asset_path: ipa_path, platform: platform)
when "osx"
package_path = FastlaneCore::PkgUploadPackageBuilder.new.generate(
app_id: Deliver.cache[:app].id,
pkg_path: pkg_path,
package_path: "/tmp",
platform: platform
)
result = transporter.upload(package_path: package_path, asset_path: pkg_path)
result = transporter.upload(package_path: package_path, asset_path: pkg_path, platform: platform)
else
UI.user_error!("No suitable file found for upload for platform: #{options[:platform]}")
end
Expand Down Expand Up @@ -270,25 +270,25 @@ 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
def transporter_for_selected_team(upload: false)
# Use JWT auth
api_token = Spaceship::ConnectAPI.token
unless api_token.nil?
api_token.refresh! if api_token.expired?
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text)
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, upload: upload, api_key: options[:api_key])
end

tunes_client = Spaceship::ConnectAPI.client.tunes_client

generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider])
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], upload: upload, api_key: options[:api_key])
return generic_transporter unless options[:itc_provider].nil? && tunes_client.teams.count > 1

begin
team = tunes_client.teams.find { |t| t['providerId'].to_s == tunes_client.team_id }
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)
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, upload: upload, api_key: options[: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
Expand Down
6 changes: 3 additions & 3 deletions deliver/spec/runner_spec.rb
Expand Up @@ -63,7 +63,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::IpaUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', ipa_path: 'ACME.ipa', package_path: '/tmp', platform: 'ios')
.and_return('path')
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.ipa').and_return(true)
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.ipa', platform: 'ios').and_return(true)
runner.upload_binary
end
end
Expand All @@ -77,7 +77,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::IpaUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', ipa_path: 'ACME.ipa', package_path: '/tmp', platform: 'appletvos')
.and_return('path')
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.ipa').and_return(true)
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.ipa', platform: 'appletvos').and_return(true)
runner.upload_binary
end
end
Expand All @@ -93,7 +93,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::PkgUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', pkg_path: 'ACME.pkg', package_path: '/tmp', platform: 'osx')
.and_return('path')
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.pkg').and_return(true)
expect(transporter).to receive(:upload).with(package_path: 'path', asset_path: 'ACME.pkg', platform: 'osx').and_return(true)
runner.upload_binary
end
end
Expand Down
1 change: 1 addition & 0 deletions fake testers_file_path
@@ -0,0 +1 @@
First,Last,Email,Groups,Installed Version,Install Date
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this file uploaded by mistake? Do we need it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed! it is autogenerated file by unit tests. i will fix it properly in separate PR 13afc8c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the unit tests generate such files in a temporary dir instead on generating them at the repo root? 🤔

Though I think that file is not generated by any test that you added in this PR, but has instead always been generated there, by another test that was already there even before your PR, right? In that case, fixing that is outside the scope of this PR; but maybe something to keep in mind for a later fix? 🤷

144 changes: 137 additions & 7 deletions fastlane_core/lib/fastlane_core/itunes_transporter.rb
Expand Up @@ -180,9 +180,123 @@ def additional_upload_parameters
end
end

# Generates commands and executes the altool.
class AltoolTransporterExecutor < TransporterExecutor
ERROR_REGEX = /\sNSLocalizedFailureReason\s=\s"+(.+)"/

private_constant :ERROR_REGEX

def execute(command, hide_output)
if Helper.test?
yield(nil) if block_given?
command
end

@errors = []
@warnings = []
@all_lines = []

if hide_output
giginet marked this conversation as resolved.
Show resolved Hide resolved
# Show a one time message instead
UI.success("Waiting for App Store Connect transporter to be finished.")
UI.success("Application Loader progress... this might take a few minutes...")
end

begin
exit_status = FastlaneCore::FastlanePty.spawn(command) do |command_stdout, command_stdin, pid|
begin
giginet marked this conversation as resolved.
Show resolved Hide resolved
command_stdout.each do |line|
@all_lines << line
parse_line(line, hide_output) # this is where the parsing happens
end
end
end
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved
rescue => ex
# FastlanePty adds exit_status on to StandardError so every error will have a status code
exit_status = ex.exit_status
@errors << ex.to_s
end

@errors << "The call to the altool completed with a non-zero exit status: #{exit_status}. This indicates a failure." unless exit_status.zero?

UI.important(@warnings.join("\n")) unless @warnings.empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Given UI.important usually prefixes the message with the time of the log, that means that the first warning in that array will be prefixed by the timestamp but not the others, leading to some misalignment in the logs. Not dramatic, but could look a bit odd.
Maybe emitting one UI.important per warning instead, so each has the timestamp prefix, would look better? And as a bonus you would not have to test for @warnings.empty? since the each loop would just never iterate if the array is empty 🙃

Suggested change
UI.important(@warnings.join("\n")) unless @warnings.empty?
@warnings.each { |warning| UI.important(warning) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as a bonus you would not have to test for @warnings.empty? since the each loop would just never iterate if the array is empty 🙃

Ah, yes, currently there is no change to parse warning here, so maybe it would be better to remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

44bf7aa fixed!


if @errors.count > 0 && @all_lines.count > 0
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved
# Print out the last 18 lines, this is key for non-verbose mode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have sample output or test cases?

We may prefer to use regex instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found good way so please wait a moment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry I think this line should be leave it as is, because the main purpose is providing altool's command output itself and regex is already done at here

if line =~ ERROR_REGEX
@errors << $1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2022-09-09 16:12:21.904 Has version info conflict: YES, Previous version info: 15
2022-09-09 16:12:21.905 *** Error: Error uploading '***.ipa'.
2022-09-09 16:12:21.905 *** Error: The provided entity includes an attribute with a value that has already been used The bundle version must be higher than the previously uploaded version: ‘15’. (ID: ***) (-19232)
 {
    NSLocalizedDescription = "The provided entity includes an attribute with a value that has already been used";
    NSLocalizedFailureReason = "The bundle version must be higher than the previously uploaded version: ***";
    "original_server_error" =     {
        code = "ENTITY_ERROR.ATTRIBUTE.INVALID.DUPLICATE";
        detail = "The bundle version must be higher than the previously uploaded version.";
        id = "***";
        meta =         {
            previousBundleVersion = 15;
        };
        source =         {
            pointer = "/data/attributes/cfBundleVersion";
        };
        status = 409;
        title = "The provided entity includes an attribute with a value that has already been used";
    };
    previousBundleVersion = 15;
}

This is a sample and I found I may can improve around error, let me fix it

@all_lines.last(18).each do |line|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this will always be the last 18 lines? What if there are other metadata in the JSON printed in altool's log that makes in take more or less lines, depending on the type of error and the original_server_error, or if the NSLocalizedDescription, NSLocalizedFailureReason, and/or title ends up being some multiline text depending on the error you encounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this will always be the last 18 lines?

Honestly, I'm not sure 😅 . I only encountered and know original_server_error error and it almost 18 line to show JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about detecting the first line with *** Error: in it, and print all the lines from that one to the end?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively, detecting each line starting with /[0-9 .:-]* \*\*\* Error:/, and for each one, accumulate all the following lines not starting with a timestamp like /[0-9 .:-]*/ — to pick up all lines from that error… up to the next timestamped log line, if any 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

01a5873
I wanna propose this improvement. This will shows the lines until ***Error shown. If ***Error cannot be found (it is super very rare case since @errors.count > 0 && @all_lines.count > 0), shows last 20 line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another tip: you can index arrays using ranges and negative indices like arr[-20..-1] for example (equivalent to last(20)) or arr[error_line_index..-1] (to get lines from index error_line_index to the end of the array)

I think using that instead of last(…your long condition + math…) would make the code more readable.

So this whole last expression would become something like @all_lines[(error_line_index || -20)..-1].each do … end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh.. cool!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you should add those example messages you commented above … as unit tests 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good. let me add unit test later 🙇 (Sorry I'm busy from now so it will be later)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, and thanks already for handling all this 👍

UI.important("[altool] #{line}")
end
UI.message("Application Loader output above ^")
UI.error(@errors.join("\n"))
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved
end

yield(@all_lines) if block_given?
exit_status.zero?
end

def build_upload_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil, platform = nil, api_key = nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💄 I'd love for this method to use named parameters at some point (build_upload_command(username:, password:, source: "/tmp", provider_short_name: "", jwt: nil, platform: nil, api_key: nil) for easier-to-read call sites; but that's just a code-style nitpick and not a blocker at all, and can be ironed out in a separate PR, so no worries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, since build_upload_command's style is same with other executer class so it would be kind of refactoring. I think I can do it later PR 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally 👍 no need to fix this in this PR given the refactoring it will involve indeed. And not that critical if we don't do it anyway to be honest 🙃

giginet marked this conversation as resolved.
Show resolved Hide resolved
[
("API_PRIVATE_KEYS_DIR=#{api_key[:key_filepath]}" if api_key),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • we are always expecting key_filepath which will break all the CI because app_store_connect_api_key action supports api key_content inside the ENV variable (the content of the .p8 file) also instead of passing the .p8 file-path.
  • Please see the last part of this document https://docs.fastlane.tools/app-store-connect-api/
  • i think, we should find a way to handle that scenario also. maybe we need to create .p8 file from key_content 🙈
  • Adding a simple example to test this scenario in which app_store_connect_api_key accepts the key_content instead of p8 file-path
default_platform(:ios)

platform :ios do
  desc "Description of what the lane does"
  lane :custom_lane do
    xcode_select("/Applications/Xcode_14 beta 6.app")
    api_key = app_store_connect_api_key(
      key_id: "XXXXXX",
      issuer_id: "XXX-XXX-XXXX",
      key_content: "-----BEGIN PRIVATE KEY-----\nXXXXXXX\nXXXXXX\nXXXXXXXX\nXXX\n-----END PRIVATE KEY-----",
      duration: 1200,
      in_house: false
    )    
    
    pilot(api_key: api_key)
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... I missed this case, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think, we should find a way to handle that scenario also. maybe we need to create .p8 file from key_content 🙈

yes! it is solution for this case, let me do so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed key_filepath is not existed and i need to do other way, I'll push commit soon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

747178f fixed

"xcrun altool",
"--upload-app",
("-u #{username.shellescape}" unless api_key),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @freddi-kit

Nice work! i am just testing this PR and it got crashed in this line shellescape
Also, the username is indeed nil in my case because i am using API-Key inside pilot so technially this line should not even execute. i think, maybe we are not passing the api_key correct way in this method....

3: from /Users/distiller/project/vendor/bundle/ruby/2.7.0/bundler/gems/fastlane-ed28c906445c/fastlane/lib/fastlane/actions/upload_to_testflight.rb:34:in `run'
	 2: from /Users/distiller/project/vendor/bundle/ruby/2.7.0/bundler/gems/fastlane-ed28c906445c/pilot/lib/pilot/build_manager.rb:50:in `upload'
	 1: from /Users/distiller/project/vendor/bundle/ruby/2.7.0/bundler/gems/fastlane-ed28c906445c/fastlane_core/lib/fastlane_core/itunes_transporter.rb:697:in `upload'
/Users/distiller/project/vendor/bundle/ruby/2.7.0/bundler/gems/fastlane-ed28c906445c/fastlane_core/lib/fastlane_core/itunes_transporter.rb:242:in `build_upload_command': undefined method `shellescape' for nil:NilClass (NoMethodError)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, pilot + username(apple-id), password is working well 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh Thank you for reviewing!

in my environment, pilot+api_key works. and here is my example Fastfile.
How is mine and your Fastfile different?

default_platform(:ios)

platform :ios do
  desc "Description of what the lane does"
  lane :custom_lane do
    xcode_select("/Applications/Xcode_14 beta 6.app")
    api_key = app_store_connect_api_key(
      key_id: "XXXXXX",
      issuer_id: "XXX-XXX-XXXX",
      key_filepath: "./AuthKey_XXXX.p8",
      duration: 1200,
      in_house: false
    )    
    
    pilot(api_key: api_key)
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... sorry I may found there is a issue to check password is available or not, please wait a moment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this is simple issue comes from my less experience Ruby 🙇 I can fix it soon

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @freddi-kit! Thank you so much for working on that.

I second @raid5 comment. I believe there's an unhandled case when using api_key_path (instead of api_key):

fastlane_core/lib/fastlane_core/itunes_transporter.rb:241:in build_upload_command': undefined method shellescape' for nil:NilClass (NoMethodError)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just investigated and it is bug, and it requires a little more changes to support it api_key_path. I'm happy if you wait for it 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! 22cfcec

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome this works great! I was able to successfully upload to TestFlight now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same! Thank you so much @freddi-kit. As others said:

Some heroes don't wear capes, @freddi-kit 😁

("-p #{password.shellescape}" unless api_key),
("--apiKey #{api_key[:key_id]}" if api_key),
("--apiIssuer #{api_key[:issuer_id]}" if api_key),
platform_option(platform),
file_upload_option(source),
additional_upload_parameters,
"-k 100000"
].compact.join(' ')
end

def additional_upload_parameters
env_deliver_additional_params = ENV["DELIVER_ALTOOL_ADDITIONAL_UPLOAD_PARAMETERS"]
if env_deliver_additional_params.to_s.strip.empty?
return nil
end
giginet marked this conversation as resolved.
Show resolved Hide resolved
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved

return env_deliver_additional_params.to_s.strip
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved
end

def handle_error(password)
UI.error("Could not download/upload from App Store Connect!")
giginet marked this conversation as resolved.
Show resolved Hide resolved
end

def displayable_errors
@errors.map { |error| "[Application Loader Error Output]: #{error}" }.join("\n")
end

private

def file_upload_option(source)
"-f #{source.shellescape}"
end

def platform_option(platform)
"-t #{platform == 'osx' ? 'macox' : platform}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

Suggested change
"-t #{platform == 'osx' ? 'macox' : platform}"
"-t #{platform == 'osx' ? 'macosx' : platform}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, since that typo didn't make the test suite fail… maybe that's a sign that we need to add some test case (aka spec example) to our test suite for it? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this brings to point that are we really sure we need to make it macosx from osx?

Apple's official document is saying osx should be enough which means we don't require this condition at all, right? maybe @freddi-kit knows? 👀
https://help.apple.com/itc/apploader/#/apdATD1E53-D1E1A1303-D1E53A1126

Screenshot 2022-09-10 at 21 18 16

maybe that's a sign that we need to add some test case (aka spec example) to our test suite for it? 🤔

i agree!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and when i m running xcrun altool --help in terminal then Apple is saying it should be macos 🙈
so should we use osx or macosx or macos? 😅🤔

To be honest, i have no mac-app to upload and verify about it...

Screenshot 2022-09-10 at 21 35 09

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be macos and it is my mistake 🙇 let me fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, i have no mac-app to upload and verify about it...

I just tested with new empty mac os app and it success to upload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9988e33 fixed

end

def parse_line(line, hide_output)
output_done = false

if line =~ ERROR_REGEX
@errors << $1
output_done = true
end

unless hide_output
# General logging for debug purposes
unless output_done
UI.verbose("[altool]: #{$1}")
end
end
end
end

# Generates commands and executes the iTMSTransporter through the shell script it provides by the same name
class ShellScriptTransporterExecutor < TransporterExecutor
def build_upload_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
def build_upload_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil, platform = nil, api_key = nil)
use_jwt = !jwt.to_s.empty?
[
'"' + Helper.transporter_path + '"',
Expand Down Expand Up @@ -278,7 +392,7 @@ def shell_escaped_password(password)
# Generates commands and executes the iTMSTransporter by invoking its Java app directly, to avoid the crazy parameter
# escaping problems in its accompanying shell script.
class JavaTransporterExecutor < TransporterExecutor
def build_upload_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
def build_upload_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil, platform = nil, api_key = nil)
use_jwt = !jwt.to_s.empty?
if !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(11)
[
Expand Down Expand Up @@ -476,7 +590,7 @@ def self.hide_transporter_output?
# see: https://github.com/fastlane/fastlane/issues/1524#issuecomment-196370628
# for more information about how to use the iTMSTransporter to list your provider
# short names
def initialize(user = nil, password = nil, use_shell_script = false, provider_short_name = nil, jwt = nil)
def initialize(user = nil, password = nil, use_shell_script = false, provider_short_name = nil, jwt = nil, upload: false, api_key: nil)
# Xcode 6.x doesn't have the same iTMSTransporter Java setup as later Xcode versions, so
# we can't default to using the newer direct Java invocation strategy for those versions.
use_shell_script ||= Helper.is_mac? && Helper.xcode_version.start_with?('6.')
Expand All @@ -489,8 +603,16 @@ def initialize(user = nil, password = nil, use_shell_script = false, provider_sh
end

@jwt = jwt
@api_key = api_key

if should_use_altool?(upload, use_shell_script)
UI.verbose("Use altool as transporter.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Using makes more sense here:

Suggested change
UI.verbose("Use altool as transporter.")
UI.verbose("Using altool as transporter.")

@transporter_executor = AltoolTransporterExecutor.new
else
UI.verbose("Use iTMSTransporter as transporter")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UI.verbose("Use iTMSTransporter as transporter")
UI.verbose("Using iTMSTransporter as transporter.")

@transporter_executor = use_shell_script ? ShellScriptTransporterExecutor.new : JavaTransporterExecutor.new
end

@transporter_executor = use_shell_script ? ShellScriptTransporterExecutor.new : JavaTransporterExecutor.new
@provider_short_name = provider_short_name
end

Expand Down Expand Up @@ -539,7 +661,7 @@ def download(app_id, dir = nil)
# @return (Bool) True if everything worked fine
# @raise [Deliver::TransporterTransferError] when something went wrong
# when transferring
def upload(app_id = nil, dir = nil, package_path: nil, asset_path: nil)
def upload(app_id = nil, dir = nil, package_path: nil, asset_path: nil, platform: nil)
raise "app_id and dir are required or package_path or asset_path is required" if (app_id.nil? || dir.nil?) && package_path.nil? && asset_path.nil?

# Transport can upload .ipa, .dmg, and .pkg files directly with -assetFile
Expand Down Expand Up @@ -568,9 +690,11 @@ def upload(app_id = nil, dir = nil, package_path: nil, asset_path: nil)

password_placeholder = @jwt.nil? ? 'YourPassword' : nil
jwt_placeholder = @jwt.nil? ? nil : 'YourJWT'
api_key_plaseholder = nil unless @api_key.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
api_key_plaseholder = nil unless @api_key.nil?
api_key_placeholder = nil unless @api_key.nil?

please fix this spell in other places too...

api_key_plaseholder = { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_filepath: "YourKeyFilepath" } if @api_key.nil?

command = @transporter_executor.build_upload_command(@user, @password, actual_dir, @provider_short_name, @jwt)
UI.verbose(@transporter_executor.build_upload_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt_placeholder))
command = @transporter_executor.build_upload_command(@user, @password, actual_dir, @provider_short_name, @jwt, platform, @api_key)
UI.verbose(@transporter_executor.build_upload_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt_placeholder, platform, api_key_plaseholder))

begin
result = @transporter_executor.execute(command, ItunesTransporter.hide_transporter_output?)
Expand Down Expand Up @@ -657,6 +781,12 @@ def provider_ids

TWO_FACTOR_ENV_VARIABLE = "FASTLANE_APPLE_APPLICATION_SPECIFIC_PASSWORD"

# Returns whether altool should be used or ItunesTransporter should be used
def should_use_altool?(upload, use_shell_script)
# Xcode 14 no longer supports iTMSTransporter. Use altool instead
!use_shell_script && upload && !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(14)
end

# Returns the password to be used with the transporter
def load_password_for_transporter
# 3 different sources for the password
Expand Down
8 changes: 4 additions & 4 deletions pilot/lib/pilot/build_manager.rb
Expand Up @@ -47,7 +47,7 @@ def upload(options)
end

transporter = transporter_for_selected_team(options)
result = transporter.upload(package_path: package_path, asset_path: asset_path)
result = transporter.upload(package_path: package_path, asset_path: asset_path, platform: platform == "osx" ? "macos" : platform)
freddi-kit marked this conversation as resolved.
Show resolved Hide resolved

unless result
transporter_errors = transporter.displayable_errors
Expand Down Expand Up @@ -391,13 +391,13 @@ def transporter_for_selected_team(options)
api_token = Spaceship::ConnectAPI.token
unless api_token.nil?
api_token.refresh! if api_token.expired?
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text)
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, upload: true, api_key: options[:api_key])
end

# Otherwise use username and password
tunes_client = Spaceship::ConnectAPI.client ? Spaceship::ConnectAPI.client.tunes_client : nil

generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider])
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], upload: true, api_key: options[:api_key])
return generic_transporter if options[:itc_provider] || tunes_client.nil?
return generic_transporter unless tunes_client.teams.count > 1

Expand All @@ -406,7 +406,7 @@ def transporter_for_selected_team(options)
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)
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, upload: true, api_key: options[:api_key])
rescue => ex
STDERR.puts(ex.to_s)
UI.verbose("Couldn't infer a provider short name for team with id #{tunes_client.team_id} automatically: #{ex}. Proceeding without provider short name.")
Expand Down