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

[regression][pilot] Fix upload using api_key_path + apple_id input options #18771

Merged
merged 10 commits into from
Jun 6, 2021

Conversation

crazymanish
Copy link
Member

@crazymanish crazymanish commented May 29, 2021

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've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

More details:

  • Pilot don't login when apple_id input option is given...please see here
  • Also, CLI api_key_path input param does not set Spaceship::ConnectAPI.token same as app_store_connect_api_key action

Now, the problem is: When pilot is trying to upload binary, there is no Token for ItunesTransporter, and pilot is not trying to generate a new token using the given api_key_path input option which is exactly happening in #18767
and
User doesn't see this problem when apple_id not given because in that case, pilot always do login and sets the Spaceship::ConnectAPI.token at the starting itself...see here

Description

  • Fix the above regression

Testing Steps

  • Update Gemfile and run bundle install with
gem "fastlane", :git => "git@github.com:fastlane/fastlane.git", :branch => "crazymanish-pilot-token-creation-fix"
  • Try pilot's upload using CLI
bundle exec fastlane pilot upload --api_key_path /Users/runner/work/_temp/api_key***.json -p 1528xxxxxx`

@google-cla google-cla bot added the cla: yes label May 29, 2021
@crazymanish
Copy link
Member Author

Ubuntu failing unit-tests is so random and created little noise in this PR git-commits 😅
Also,
Fixed this Ubuntu failing unit-test for always here #18778 😊

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Thanks for making this fix! This does fix it but I think the root of this issue is that the login method isn’t being called after any of the Pilot::BuildManager.new calls 🤔 in https://github.com/fastlane/fastlane/blob/master/pilot/lib/pilot/commands_generator.rb

I think if we add the login call that this should fix the issue without duplicating any code for the token creation 🤷‍♂️ Thoughts?!

@crazymanish
Copy link
Member Author

Thanks for making this fix! This does fix it but I think the root of this issue is that the login method isn’t being called after any of the Pilot::BuildManager.new calls 🤔 in https://github.com/fastlane/fastlane/blob/master/pilot/lib/pilot/commands_generator.rb

I think if we add the login call that this should fix the issue without duplicating any code for the token creation 🤷‍♂️ Thoughts?!

Yes, login call will fix this issue...I was a little confused earlier should we reuse the login call or duplicate only Spaceship token code :)
Please feel free to review!

Comment on lines 369 to 370
# Ensure that `Spaceship::ConnectAPI.token` is set correctly, if required
login
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of here, I am thinking can we start upload with login always in this line?

# from this...

# Only need to login before upload if no apple_id was given      
# 'login' will be deferred until before waiting for build processing      
should_login_in_start = options[:apple_id].nil?      
start(options, should_login: should_login_in_start)

# to this...
start(options, should_login: true)

# If we go with approach then the login code will be in one place and maintenance will be easy too... 
# I also notice, Deliver always do login on start so it will bring consistency too 😇

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

I deleted my thought because it was in your comment 😛 I’m going to make that change, push it out, and release it!

@google-cla
Copy link

google-cla bot commented Jun 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 5, 2021
@google-cla
Copy link

google-cla bot commented Jun 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@joshdholtz
Copy link
Member

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 5, 2021
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is 🔥 now! Took a lot to get here but thank you for working through this ❤️

@joshdholtz joshdholtz merged commit b27eb04 into master Jun 6, 2021
@joshdholtz joshdholtz deleted the crazymanish-pilot-token-creation-fix branch June 6, 2021 00:03
Comment on lines +369 to +370
# Ensure that user is authenticated
start(options)
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @joshdholtz

I think, this will not perform login and will early exit in this line see here
because
@ config is already set by the upload method without login when apple_id is given

Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

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

Congratulations! 🎉 This was released as part of fastlane 2.185.0 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] fastlane pilot upload is failing when --apple_id (-p) is provided
3 participants