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

[match][spaceship] Fixed matchfile overriding fastfile team_id with bonus spaceship select_team additions #12235

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Apr 4, 2018

Fixes #12146
Starting implementation of #12234 (see this issue for more details and examples)

Issue

Using action options verify_block can have some unintended side effects that will break the rules of Priorities of parameters and options

  1. CLI parameter (e.g. gym --scheme Example) or Fastfile (e.g. gym(scheme: 'Example'))
  2. Environment variable (e.g. GYM_SCHEME)
  3. Tool specific config file (e.g. Gymfile containing scheme 'Example')
  4. Default value (which might be taken from the Appfile, e.g. app_identifier from the Appfile)
  5. If this value is required, you'll be asked for it (e.g. you have multiple schemes, you'll be asked for it)

Example of how this happens

This is mainly a problem when load_configuration_file is called outside of standard action loading process -

class SyncCodeSigningAction < Action
def self.run(params)
require 'match'
params.load_configuration_file("Matchfile")
Match::Runner.new.run(params)
define_profile_type(params)
define_provisioning_profile_mapping(params)
end

In match's case above, it will load...

  1. Load the Fastfile parameter (this is correct)
  2. Attempt to load the Matchfile parameter but doesn't because Fastfile parameter set
  3. During match's run, this extra load_configuration_file will attempt to load files from Matchfile again (and won't override the Fastfile value) but the verify_block method gets called and sets ENV["FASTLANE_TEAM_ID"] to a potentially different team id (if the Matchfile value is different than the Fastfile value) which will cause a different value to be shown in match's summary than what actually get's used by spaceship

Solution

  1. verify_block should not be used to set environment variables verify_block can actually get called multiple times and should really only be used for verifying a value (not setting it)
  2. We should stop setting environment variables for FASTLANE_TEAM_ID, FASTLANE_TEAM_NAME, FASTLANE_ITC_TEAM_ID, and FASTLANE_ITC_TEAM_NAME for to use with the spaceship's select_team method
  3. select_team should take a parameter for team_id and team_name when explicitly looking for a team instead of setting a global environment variable that may potentially get overridden

@KrauseFx
Copy link
Member

KrauseFx commented Apr 5, 2018

Make sure to update the PR title and also update it when hitting that merge button

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.

I haven't tested it, but the code looks 💯

@joshdholtz joshdholtz changed the title [match][spaceship] Fixed matchfile overriding fastfile team_id with b… [match][spaceship] Fixed matchfile overriding fastfile team_id with bonus spaceship select_team additions Apr 5, 2018
@joshdholtz joshdholtz merged commit 85f8a17 into master Apr 9, 2018
@joshdholtz joshdholtz deleted the joshdholtz-fix-match-option-loading-and-spaceship-team-select branch April 9, 2018 18:26
@fastlane-bot
Copy link

Hey @joshdholtz 👋

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.90.0 🚀

@fastlane fastlane locked and limited conversation to collaborators Jun 10, 2018
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