-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] Implements verify
with altool
for Xcode 14 validation
#20738
Changes from 8 commits
73d0982
b429756
8fd629f
f6bc50a
af7f6f9
7aa1045
c8b568b
e609699
cf14c9e
7af747f
e046af1
d761225
c1bc3e8
7f1dcde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,7 +169,7 @@ def verify_binary | |
pkg_path = options[:pkg] | ||
|
||
platform = options[:platform] | ||
transporter = transporter_for_selected_team | ||
transporter = transporter_for_selected_team(altool_compatible_command: true) | ||
|
||
case platform | ||
when "ios", "appletvos" | ||
|
@@ -179,15 +179,15 @@ def verify_binary | |
package_path: "/tmp", | ||
platform: platform | ||
) | ||
result = transporter.verify(package_path: package_path) | ||
result = transporter.verify(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.verify(package_path: package_path) | ||
result = transporter.verify(package_path: package_path, asset_path: pkg_path, platform: platform) | ||
else | ||
UI.user_error!("No suitable file found for verify for platform: #{options[:platform]}") | ||
end | ||
|
@@ -206,7 +206,7 @@ def upload_binary | |
pkg_path = options[:pkg] | ||
|
||
platform = options[:platform] | ||
transporter = transporter_for_selected_team(upload: true) | ||
transporter = transporter_for_selected_team(altool_compatible_command: true) | ||
|
||
case platform | ||
when "ios", "appletvos" | ||
|
@@ -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) | ||
# Use JWT auth | ||
api_token = Spaceship::ConnectAPI.token | ||
api_key = if options[:api_key].nil? && !api_token.nil? | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can always pass |
||
end | ||
|
||
tunes_client = Spaceship::ConnectAPI.client.tunes_client | ||
|
||
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], upload: upload, api_key: api_key) | ||
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], altool_compatible_command: altool_compatible_command, api_key: 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, upload: upload, api_key: api_key) | ||
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, altool_compatible_command: altool_compatible_command, 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 | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nits] I think it's better to use
But this point is not important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a very good point, I have just modified the abstract method to use |
||||||||
not_implemented(__method__) | ||||||||
end | ||||||||
|
||||||||
|
@@ -307,8 +307,20 @@ def build_download_command(username, password, apple_id, destination = "/tmp", p | |||||||
raise "This feature has not been implemented yet with altool for Xcode 14" | ||||||||
end | ||||||||
|
||||||||
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil) | ||||||||
raise "This feature has not been implemented yet with altool for Xcode 14" | ||||||||
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil, platform = nil, api_key = nil) | ||||||||
use_api_key = !api_key.nil? | ||||||||
[ | ||||||||
("API_PRIVATE_KEYS_DIR=#{api_key[:key_dir]}" if use_api_key), | ||||||||
"xcrun altool", | ||||||||
"--validate-app", | ||||||||
("-u #{username.shellescape}" unless use_api_key), | ||||||||
("-p #{password.shellescape}" unless use_api_key), | ||||||||
("--apiKey #{api_key[:key_id]}" if use_api_key), | ||||||||
("--apiIssuer #{api_key[:issuer_id]}" if use_api_key), | ||||||||
("--asc-provider #{provider_short_name}" unless use_api_key || provider_short_name.to_s.empty?), | ||||||||
platform_option(platform), | ||||||||
file_upload_option(source) | ||||||||
].compact.join(' ') | ||||||||
end | ||||||||
|
||||||||
def additional_upload_parameters | ||||||||
|
@@ -409,7 +421,7 @@ def build_provider_ids_command(username, password, jwt = nil, api_key = nil) | |||||||
].compact.join(' ') | ||||||||
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) | ||||||||
use_jwt = !jwt.to_s.empty? | ||||||||
[ | ||||||||
'"' + Helper.transporter_path + '"', | ||||||||
|
@@ -504,7 +516,7 @@ def build_upload_command(username, password, source = "/tmp", provider_short_nam | |||||||
end | ||||||||
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) | ||||||||
use_jwt = !jwt.to_s.empty? | ||||||||
if !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(11) | ||||||||
[ | ||||||||
|
@@ -660,7 +672,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, upload: false, api_key: nil) | ||||||||
def initialize(user = nil, password = nil, use_shell_script = false, provider_short_name = nil, jwt = nil, altool_compatible_command: 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.') | ||||||||
|
@@ -675,7 +687,7 @@ def initialize(user = nil, password = nil, use_shell_script = false, provider_sh | |||||||
@jwt = jwt | ||||||||
@api_key = api_key | ||||||||
|
||||||||
if should_use_altool?(upload, use_shell_script) | ||||||||
if should_use_altool?(altool_compatible_command, use_shell_script) | ||||||||
UI.verbose("Using altool as transporter.") | ||||||||
@transporter_executor = AltoolTransporterExecutor.new | ||||||||
else | ||||||||
|
@@ -800,10 +812,20 @@ def upload(app_id = nil, dir = nil, package_path: nil, asset_path: nil, platform | |||||||
# @return (Bool) True if everything worked fine | ||||||||
# @raise [Deliver::TransporterTransferError] when something went wrong | ||||||||
# when transferring | ||||||||
def verify(app_id = nil, dir = nil, package_path: nil) | ||||||||
raise "Either a combination of app id and directory or a package_path are required" if (app_id.nil? || dir.nil?) && package_path.nil? | ||||||||
def verify(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? | ||||||||
|
||||||||
force_itmsp = FastlaneCore::Env.truthy?("ITMSTRANSPORTER_FORCE_ITMS_PACKAGE_UPLOAD") | ||||||||
can_use_asset_path = Helper.is_mac? && asset_path | ||||||||
|
||||||||
actual_dir = if package_path | ||||||||
actual_dir = if can_use_asset_path && !force_itmsp | ||||||||
# The asset gets deleted upon completion so copying to a temp directory | ||||||||
# (with randomized filename, for multibyte-mixed filename upload fails) | ||||||||
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 | ||||||||
giginet marked this conversation as resolved.
Show resolved
Hide resolved
giginet marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
elsif package_path | ||||||||
package_path | ||||||||
else | ||||||||
File.join(dir, "#{app_id}.itmsp") | ||||||||
|
@@ -812,8 +834,15 @@ def verify(app_id = nil, dir = nil, package_path: nil) | |||||||
password_placeholder = @jwt.nil? ? 'YourPassword' : nil | ||||||||
jwt_placeholder = @jwt.nil? ? nil : 'YourJWT' | ||||||||
|
||||||||
command = @transporter_executor.build_verify_command(@user, @password, actual_dir, @provider_short_name, @jwt) | ||||||||
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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to be commented for code readability 👍
Suggested change
|
||||||||
|
||||||||
api_key = nil | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unnecessary
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed 👍 |
||||||||
api_key = api_key_with_p8_file_path(@api_key) if use_api_key | ||||||||
|
||||||||
command = @transporter_executor.build_verify_command(@user, @password, actual_dir, @provider_short_name, @jwt, platform, api_key) | ||||||||
UI.verbose(@transporter_executor.build_verify_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt_placeholder, platform, api_key_placeholder)) | ||||||||
|
||||||||
begin | ||||||||
result = @transporter_executor.execute(command, ItunesTransporter.hide_transporter_output?) | ||||||||
|
@@ -883,9 +912,9 @@ def api_key_with_p8_file_path(original_api_key) | |||||||
end | ||||||||
|
||||||||
# Returns whether altool should be used or ItunesTransporter should be used | ||||||||
def should_use_altool?(upload, use_shell_script) | ||||||||
def should_use_altool?(altool_compatible_command, 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) | ||||||||
!use_shell_script && altool_compatible_command && !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(14) | ||||||||
end | ||||||||
|
||||||||
# Returns the password to be used with the transporter | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the situation
altool_compatible_command
to befalse
?I think this flag is not necessary for
ItunesTransporter
.In the previous implementation,
verify
task doesn't usealtool
, 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will still be needed as not all commands have
altool
implementations. The reason why theupload
property existed was so that even if the conditions were met onlyupload
would useitmstransporter
, 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 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant all callers of this method seem to pass
true
foraltool_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.