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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[action][ensure_env_vars] efficiency improvement #18522

Merged
merged 3 commits into from Apr 12, 2021

Conversation

crazymanish
Copy link
Member

@crazymanish crazymanish commented Apr 8, 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

  • Problem: If multiple ENV variables are missing then the user has to run ensure_env_vars multiple times because it always EXIT after found the first missing ENV variable.
  • This is slightly inconvenience because Fastlane uses lots of ENV variables and good to report all expected missing ENV variables after one run itself to save time and to avoid Fastlane run ensure_env_vars multiple times 馃槆

Description

  • Made ensure_env_vars more efficient, now it will report all missing ENV variables after one run itself.
  • Please see the screenshot for more details

Testing Steps

  • Update Gemfile and run bundle install with
gem "fastlane", :git => "git@github.com:crazymanish/fastlane.git", :branch => "ensure-env-vars-improvement"
  • With the below test lane, run bundle exec fastlane test
lane :test do
    #ENV["GITHUB_API_TOKEN_MISSING_EXAMPLE"] = 'xxxxx-xxxxxxx-xxxxxxx'
    ENV["GYM_SCHEME"] = "MyApp"
    ENV["GYM_EXPORT_METHOD"] = 'app-store'
    #ENV["GYM_CLEAN"] = 'true'
    ENV["GYM_SILENT"] = 'true'
    #ENV["SLACK_API_TOKEN_MISSING_EXAMPLE"] = 'xxxxx-xxxxxxx-xxxxxxx'

    ensure_env_vars(
      env_vars: [
        'GITHUB_API_TOKEN_MISSING_EXAMPLE',
        'GYM_SCHEME',
        'GYM_EXPORT_METHOD',
        'GYM_CLEAN',
        'GYM_SILENT',
        'SLACK_API_TOKEN_MISSING_EXAMPLE'
      ]
    )
  end

Screenshot

Screenshot 2021-04-08 at 20 26 22

@google-cla google-cla bot added the cla: yes label Apr 8, 2021
Comment on lines 8 to 9
is_one = missing_variables.length == 1
UI.user_error!("Missing environment variable#{is_one ? '' : 's'} '#{missing_variables.join('\', \'')}'") unless missing_variables.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_one = missing_variables.length == 1
UI.user_error!("Missing environment variable#{is_one ? '' : 's'} '#{missing_variables.join('\', \'')}'") unless missing_variables.empty?
UI.user_error!("Missing environment variable(s) '#{missing_variables.join('\', \'')}'") unless missing_variables.empty?

I understand that this logic comes from the below line but isn't this enough here? 馃檪 (and reusing the same local variable for a different purpose doesn't sound good in general).

@crazymanish crazymanish requested a review from ainame April 11, 2021 06:15
Copy link
Contributor

@ainame ainame left a comment

Choose a reason for hiding this comment

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

馃殌

@ainame ainame requested a review from joshdholtz April 12, 2021 03:29
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 馃敟 Thanks for fixing this! Really appreciate all of your contributions 鉂わ笍

@joshdholtz joshdholtz merged commit 970f795 into fastlane:master Apr 12, 2021
@crazymanish crazymanish deleted the ensure-env-vars-improvement branch April 12, 2021 18:52
@fastlane-bot
Copy link

Hey @crazymanish 馃憢

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 馃殌

@fastlane-bot
Copy link

Congratulations! 馃帀 This was released as part of fastlane 2.181.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.

None yet

4 participants