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

Pilot: Allow '~' in ipa path #9481

Merged
merged 5 commits into from
Jun 22, 2017

Conversation

nickffox
Copy link
Contributor

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

Fixes #9480

Copy link
Contributor

@ohayon ohayon 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 the PR @NicholasFFox! This is definitely a great change! I just left one comment about the methods in the IpaFileAnalyser class. Let me know what you think? 🐙

@@ -19,6 +19,7 @@ def self.fetch_app_version(path)

# Fetches the app platform from the given ipa file.
def self.fetch_app_platform(path)
path = File.expand_path(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be happening in the methods above too? Or maybe at the call site for all of these methods?

Copy link
Contributor Author

@nickffox nickffox Jun 14, 2017

Choose a reason for hiding this comment

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

@ohayon I really wanted to make the change in fastlane_core/lib/fastlane_core/configuration/config_item.rb by changing auto_convert_value to do this:

    def auto_convert_value(value)
      return nil if value.nil?

      if data_type == Array
        return value.split(',') if value.kind_of?(String)
      elsif data_type == Integer
        return value.to_i if value.to_i.to_s == value.to_s
      elsif data_type == Float
        return value.to_f if value.to_f.to_s == value.to_s
      elsif data_type == String && allow_shell_conversion
        return value.map(&:to_s).map(&:shellescape).join(' ') if value.kind_of?(Array)
        return value.map { |k, v| "#{k.to_s.shellescape}=#{v.shellescape}" }.join(' ') if value.kind_of?(Hash)

      # new case here
      elsif data_type == String && value.include?("~/")
        return File.expand_path(value)

      else
        # Special treatment if the user specified true, false or YES, NO
        # There is no boolean type, so we just do it here
        if %w(YES yes true TRUE).include?(value)
          return true
        elsif %w(NO no false FALSE).include?(value)
          return false
        end
      end

      return value # fallback to not doing anything
    end

That fix broke a number of tests though that had ~/ in various paths, so I opted for the less global fix.

@nickffox
Copy link
Contributor Author

@ohayon updated all methods in IpaFileAnalyser to expand paths.

Copy link
Contributor

@ohayon ohayon left a comment

Choose a reason for hiding this comment

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

Hey @NicholasFFox, thanks for making those changes! And sorry I am asking one more thing, but I'm curious to hear your thoughts on the one comment I left. 💯

@@ -14,8 +14,9 @@ def upload(options)
dir = Dir.mktmpdir

platform = fetch_app_platform
ipa_path = File.expand_path(config[:ipa])
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance you would want to move this into the generate method on IpaUploadPackageBuilder to let that method handle the expansion?

@nickffox
Copy link
Contributor Author

@ohayon completely agree with the comment and made the requested change 👍

@KrauseFx
Copy link
Member

Hey everyone, randomly jumping in here. I agree with @ohayon, this ends up with lots of duplicate code. An easy solution for that would be to do it once in gym/manager.rb right after printing the tables (as we don't necessarily want to display a really long path in the summary table) by setting it once:

Gym.config[:ipa_path] = File.expand_path(Gym.config[:ipa_path])

or similar :)

@nickffox
Copy link
Contributor Author

@KrauseFx @ohayon - I reverted my previous changes (with the exception of the ipa parameter's verify block), and added a create_config method in Pilot's CommandsGenerator. This offers a central place to perform similar operations on Pilot parameters in the future. Does this address the previous concerns?

Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

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

Looks 💯

@nickffox
Copy link
Contributor Author

@ohayon anything else that I need to do here?

@KrauseFx KrauseFx merged commit 8e4b71a into fastlane:master Jun 22, 2017
@KrauseFx
Copy link
Member

Thanks for reminding us @NicholasFFox, this is merged now and will be in the next release 👍 Thanks for contributing.

@fastlane-bot
Copy link

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

nickffox pushed a commit to nickffox/fastlane that referenced this pull request Jun 28, 2017
* Reverts most of fastlane#9481, but implements same (and improved because it catches more cases) behavior in a better place.
* Fixes fastlane#9480
@nickffox nickffox mentioned this pull request Jun 28, 2017
4 tasks
taquitos pushed a commit that referenced this pull request Jun 28, 2017
* Pilot: Refactor ipa path expansion

* Reverts most of #9481, but implements same (and improved because it catches more cases) behavior in a better place.
* Fixes #9480

* added back create_config in pilot/commands_generator
dvdchr pushed a commit to dvdchr/fastlane that referenced this pull request Sep 5, 2017
* Pilot: Allow '~' in ipa path

* added path expansion to additional methods in IpaFileAnalyser

* moved path expansion into IpaUploadPackageBuilder's generate method

* removed extra symbol that snuck in

* moved path expansion to pilot's commands_generator
dvdchr pushed a commit to dvdchr/fastlane that referenced this pull request Sep 5, 2017
* Pilot: Refactor ipa path expansion

* Reverts most of fastlane#9481, but implements same (and improved because it catches more cases) behavior in a better place.
* Fixes fastlane#9480

* added back create_config in pilot/commands_generator
@fastlane fastlane locked and limited conversation to collaborators Sep 21, 2017
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.

None yet

5 participants