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

Fix issue that uploading aab with mapping file #12856

Merged
merged 3 commits into from Jul 22, 2018

Conversation

Projects
None yet
5 participants
@chibatching
Contributor

chibatching commented Jul 6, 2018

Fixes #12900

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

When I uploaded app bundle file with mapping.txt, I got error Google Api Error: Invalid request.

FastlaneCore::Interface::FastlaneError: [!] Google Api Error: Invalid request
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane_core/lib/fastlane_core/ui/interface.rb:141:in `user_error!'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane_core/lib/fastlane_core/ui/ui.rb:17:in `method_missing'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/client.rb:390:in `rescue in call_google_api'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/client.rb:387:in `call_google_api'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/client.rb:228:in `upload_mapping'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/uploader.rb:136:in `block in upload_binaries'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/uploader.rb:134:in `each'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/uploader.rb:134:in `upload_binaries'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/supply/lib/supply/uploader.rb:26:in `perform_upload'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/actions/upload_to_play_store.rb:21:in `run'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:257:in `block (2 levels) in execute_action'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/actions/actions_helper.rb:50:in `execute_action'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:236:in `block in execute_action'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:231:in `chdir'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:231:in `execute_action'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:157:in `trigger_action_by_name'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/fast_file.rb:149:in `method_missing'
  Fastfile:64:in `block (2 levels) in parsing_binding'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/lane.rb:33:in `call'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:49:in `block in execute'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:45:in `chdir'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/runner.rb:45:in `execute'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/lane_manager.rb:59:in `cruise_lane'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/command_line_handler.rb:36:in `handle'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/commands_generator.rb:107:in `block (2 levels) in run'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/commander-fastlane-4.4.6/lib/commander/command.rb:178:in `call'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/commander-fastlane-4.4.6/lib/commander/command.rb:153:in `run'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/commander-fastlane-4.4.6/lib/commander/runner.rb:476:in `run_active_command'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane_core/lib/fastlane_core/ui/fastlane_runner.rb:75:in `run!'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/commander-fastlane-4.4.6/lib/commander/delegates.rb:15:in `run!'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/commands_generator.rb:332:in `run'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/commands_generator.rb:41:in `start'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/fastlane/lib/fastlane/cli_tools_distributor.rb:108:in `take_off'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/fastlane-2.98.0/bin/fastlane:20:in `<top (required)>'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/bin/fastlane:23:in `load'
  /Users/takao-chiba/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/bin/fastlane:23:in `<top (required)>'

This is caused that supply upload mapping.txt before app bundle file.

I tested this PR with lane to supply app bundle and mapping file and got successfully upload.

    supply(
        aab: ENV["ARTIFACTS"] + '/bundle/productRelease/app.aab',
        track: "internal",
        mapping: ENV["ARTIFACTS"] + '/mapping/product/release/mapping.txt',
        skip_upload_metadata: true,
        validate_only: true
    )

Description

At upload_binaries, upload apks and mapping.txt after that upload_bundles would upload aab file.
But when we upload aab and mapping file, apk_version_codes is not supplied yet.
So, we have to upload mapping file after both apks uploadind and aab uploading.

I split upload_binaries to apk upload and mapping upload. And call mapping upload process after apk and aab uploading.

@googlebot googlebot added the cla: yes label Jul 6, 2018

# Only update tracks if we have version codes
# Updating a track with empty version codes can completely clear out a track
update_track(apk_version_codes) unless apk_version_codes.empty?
return [client.upload_bundle(aab_path)]

This comment has been minimized.

@tmtrademarked

tmtrademarked Jul 15, 2018

Contributor

Isn't updating tracks still necessary here? (Random drive by comment, but I'm curious as to why this function differs from the logic in the newly split upload_apks)

This comment has been minimized.

@joshdholtz

joshdholtz Jul 16, 2018

Contributor

Same thoughts/concern there ^

@tmtrademarked Thanks for drive by commenting 😊

This comment has been minimized.

@chibatching

chibatching Jul 17, 2018

Contributor

The original logic sequence is below.

If uploading binary is apks

  1. Upload apks
  2. Upload mapping files
  3. Update track
  4. Upload bundle (not executed)
  5. Update track (not executed because of empty apk version codes)

If uploading binary is bundle

  1. Upload apks (not executed)
  2. Upload mapping (error caused by empty apk version codes)
  3. Update track (not executed because of empty apk version codes)
  4. Upload bundle
  5. Update track

Update track should be executed after upload apks or bundle and only once.
So, I split upload mapping and updating track function to execute after uploading both apk and bundle.

This comment has been minimized.

@chibatching

chibatching Jul 17, 2018

Contributor

After fixing,

  1. Upload apks if upload type is apks
  2. Upload bundle if upload type is bundle
  3. Upload mapping
  4. Update track

I think that uploading logic order is not changed 😃

This comment has been minimized.

@joshdholtz

joshdholtz Jul 17, 2018

Contributor

@chibatching Thanks for the explanation! I think the confusing thing was this diff removed update_track but never re-added it in back in. But looking at the whole file it looks like it its actually in the upload_mapping method - https://github.com/chibatching/fastlane/blob/49e2b1d6373efe9156ad7a3ad82932c043bf30fc/supply/lib/supply/uploader.rb#L146-L148

Thoughts on moving that to line 30 under the call to upload_mapping? I think that would make things easier to follow in this file since I don't think it has anything to do with mapping 😊

This comment has been minimized.

@chibatching

chibatching Jul 17, 2018

Contributor

Thank you comments and I agree with you 😄
I fixed at b7e91f0!

This comment has been minimized.

@chibatching

chibatching Jul 21, 2018

Contributor

@joshdholtz Could you review this?

# Only update tracks if we have version codes
# Updating a track with empty version codes can completely clear out a track
update_track(apk_version_codes) unless apk_version_codes.empty?
return [client.upload_bundle(aab_path)]

This comment has been minimized.

@joshdholtz

joshdholtz Jul 16, 2018

Contributor

Same thoughts/concern there ^

@tmtrademarked Thanks for drive by commenting 😊

return apk_version_codes
end

def upload_binaries(apk_version_codes)

This comment has been minimized.

@joshdholtz

joshdholtz Jul 16, 2018

Contributor

Can we rename this to upload_mapping? "mapping" seems more appropriate then "binary" here 🙃

This comment has been minimized.

@chibatching

chibatching Jul 17, 2018

Contributor

I fixed at 49e2b1d 😃

# Only update tracks if we have version codes
# Updating a track with empty version codes can completely clear out a track
update_track(apk_version_codes) unless apk_version_codes.empty?
return [client.upload_bundle(aab_path)]

This comment has been minimized.

@joshdholtz

joshdholtz Jul 17, 2018

Contributor

@chibatching Thanks for the explanation! I think the confusing thing was this diff removed update_track but never re-added it in back in. But looking at the whole file it looks like it its actually in the upload_mapping method - https://github.com/chibatching/fastlane/blob/49e2b1d6373efe9156ad7a3ad82932c043bf30fc/supply/lib/supply/uploader.rb#L146-L148

Thoughts on moving that to line 30 under the call to upload_mapping? I think that would make things easier to follow in this file since I don't think it has anything to do with mapping 😊

@joshdholtz

This looks great! Thanks for making those changes 😊

@joshdholtz joshdholtz merged commit 29c7eff into fastlane:master Jul 22, 2018

6 checks passed

ci/circleci: Execute tests on Ubuntu Your tests passed on CircleCI!
Details
ci/circleci: Execute tests on macOS Your tests passed on CircleCI!
Details
ci/circleci: Lint Source Code Your tests passed on CircleCI!
Details
ci/circleci: Validate Documentation Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@fastlane-bot

This comment has been minimized.

fastlane-bot commented Jul 23, 2018

Hey @chibatching 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@chibatching chibatching deleted the chibatching:fix_aab_upload_with_mapping branch Jul 23, 2018

@fastlane-bot

This comment has been minimized.

fastlane-bot commented Jul 31, 2018

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

@fastlane fastlane locked and limited conversation to collaborators Sep 30, 2018

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