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

run setup_keychain on macOS environments only #16733

Merged

Conversation

seanreinhardtapps
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

setup_travis fails on the Linux CI agents in the setup_keychain action, throwing an error and failing the task.
To run Travis CI lanes on Mac and Linux agents, setup_travis must be conditionally run.

Since that action only applies to a macOS environment, it could be updated to skip the setup_keychain step on non macOS agents.
馃攽

Description

Exit early out of setup_keychain in the setup_ci action on non mac-OS envoronments, logging a message that it was skipped.

Testing Steps

  • Run setup_ci on a macOS agent without any change in behavior.
  • Run setup_ci on a linux agent and see that it no longer fails, and logs "Skipping Keychain setup on non-macOS CI Agent"

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

鈩癸笍 Googlers: Go here for more info.

@seanreinhardtapps
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

鈩癸笍 Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jul 2, 2020
@janpio janpio added the type: ci label Jul 4, 2020
@janpio janpio requested a review from mollyIV July 4, 2020 13:24
@janpio
Copy link
Member

janpio commented Jul 4, 2020

Seems you will need some special handling in the tests as well, as they are run on Linux and Windows as well.

@mollyIV
Copy link
Member

mollyIV commented Jul 4, 2020

Hey @seanreinhardtapps , @janpio 馃憢

The setup_ci lane has been extracted as a common piece of logic for specific CI providers. It makes maintenance and future code changes easier. Here is the full context.

According to the source code of a setup_travis lane, the action is intended to be ran only on ios and mac platforms:

def self.is_supported?(platform)
[:ios, :mac].include?(platform)
end

I had a look at setup_ci code and besides setting up the keychain and setting up the output paths only for a CircleCI provider, nothing is happening.

def self.run(params)
unless should_run?(params)
UI.message("Not running on CI, skipping CI setup")
return
end
case detect_provider(params)
when 'circleci'
setup_output_paths
end
setup_keychain
end

Taking that into account, I am wondering if setup_travis should be supported on systems other than macOS right now 馃 Maybe we could do it just in time (when adding new functionality to the setup_travis action that could be supported on the macOS, Linux and Windows), not just in case.

@seanreinhardtapps could you please provide a bit more context, ideally a use-case, why and when you need to run setup_travis on an environment different than macOS?

@seanreinhardtapps
Copy link
Contributor Author

@mollyIV, @janpio thanks for the reviews on this idea.
It's not my intent to actually run setup_travis on a linux agent. My thought was that the setup_travis action could skip running on a linux agent, like it already does on a non-CI environment.

When I looked into the implementation, I thought it would make more sense to skip the setup_keychain portion since thats what fails outside of a Mac, and that expands it beyond just Travis CI.

I understand it's not usual to use a linux agent for iOS tests. I'm running a multi-stage Travis config where the last stage runs a docker container on the agent and starts XCUITests on a device farm. Thought it could be just a ruby script, it's scripted into a lane along with all of our other tasks. We had setup_travis at the top of our Fastfile before the need for the linux agent. Now its run conditionally which is fine, but I thought it would be an improvement if it did so automatically.

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 think the addition of platform checking before setting up keychain makes sense! I have a few suggestions on the code in SetupCiAction and on the tests 馃槉 But once those are fixed I think this should be 馃憤

fastlane/lib/fastlane/actions/setup_ci.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/setup_ci_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/setup_circle_ci_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/setup_travis_spec.rb Outdated Show resolved Hide resolved
@mollyIV
Copy link
Member

mollyIV commented Jul 6, 2020

I think the addition of platform checking before setting up keychain makes sense!

馃憤 馃憤

Let's also remember to change supported platforms for a setup_travis action 馃槉

def self.is_supported?(platform)
[:ios, :mac].include?(platform)
end

@joshdholtz
Copy link
Member

@seanreinhardtapps BTW, let me know if you want any help making changes to those tests! I'd be happy to hop in and help 馃槉

@seanreinhardtapps
Copy link
Contributor Author

Thanks for the reviews! PR should be ready for another look.

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 great! Thank you for making those changes 馃槉 I will get this out in a release later today for you 馃挭

@joshdholtz joshdholtz merged commit 3f134ee into fastlane:master Jul 7, 2020
@fastlane-bot
Copy link

Hey @seanreinhardtapps 馃憢

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

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

minuscorp pushed a commit to minuscorp/fastlane that referenced this pull request Jul 18, 2020
* run setup_keychain on macOS environments only

* updated test to assert results based on OS

* Swap order of checks in setup_keychain

* refactor setup_keychain tests to run consistently on all test platforms

* updated is_supported? for travis and circle_ci

* added missing test setup condition.  reorganized nesting of describes
@fastlane fastlane locked and limited conversation to collaborators Sep 6, 2020
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

6 participants