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

[action] New action to install provisioning profile from file #14516

Merged
merged 5 commits into from
May 9, 2019
Merged

[action] New action to install provisioning profile from file #14516

merged 5 commits into from
May 9, 2019

Conversation

SofteqDG
Copy link
Contributor

@SofteqDG SofteqDG commented Apr 2, 2019

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

This action can be very useful if user doesn't use match action for automatic provisioning profiles management. It can be used inside of Fastfile and also can be run from shell.
This PR also fixes open issue #11788

Description

This action allow user to install provisioning profile from local file. This is just a wrapper around the FastlaneCore::ProvisioningProfile class and doesn't provide any additional functionality.

@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 (e.g. 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.

@SofteqDG
Copy link
Contributor Author

SofteqDG commented Apr 2, 2019

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 2, 2019
@SofteqDG
Copy link
Contributor Author

SofteqDG commented Apr 2, 2019

I signed it!

@SofteqDG
Copy link
Contributor Author

SofteqDG commented Apr 9, 2019

@janpio
Hi. Do you have any other comments for this PR?

@janpio
Copy link
Member

janpio commented Apr 9, 2019

No this looks pretty good to me, just need to find the time to actually test it.

If you want to go the extra mile, having tests for this action would of course be awesome, just to make changes in the future easier.

@SofteqDG
Copy link
Contributor Author

@janpio
Yep, i can try to add tests for this action. Could you please tell me where i can see an example of how to add tests?

There is also one improvement in my head. We can expose some parameters of the provisioning profile through the lane context. I think that the most useful parameters are profile name, profile uuid, team name and team identifier. They can be used for other actions. What do you think about it?

@janpio
Copy link
Member

janpio commented Apr 15, 2019

I am not sure how one would exactly test this, but you can look at all the other files with spec in it to get an idea how testing works in general.

There is also one improvement in my head. We can expose some parameters of the provisioning profile through the lane context. I think that the most useful parameters are profile name, profile uuid, team name and team identifier. They can be used for other actions. What do you think about it?

I don't know the usecase, but could be useful. But best create a new PR for this that is separate from this (You can create a new branch in your current commit, then create your further commits there - and create a PR from that. When this is merged, the first few commits will automatically be left out of your second PR or your can rebase on master)

@SofteqDG
Copy link
Contributor Author

I am not sure how one would exactly test this, but you can look at all the other files with spec in it to get an idea how testing works in general.

Thanks. Will take a look on how testing works.

I don't know the usecase, but could be useful.

It can be used in conjunction with manual code signing. Here is an example of Fastfile:

  lane :build_production do

    # Install provisioning profile
    install_provisioning_profile(
      provisioning_profile_path: "profiles/profile.mobileprovision"
    )

    # Read provisioning profile parameters exposed by 'install_provisioning_profile' action
    profile_name = Actions.lane_context[SharedValues::PROVISIONING_PROFILE_NAME]
    prodile_uuid = Actions.lane_context[SharedValues::PROVISIONING_PROFILE_UUID]
    profile_team_id = Actions.lane_context[SharedValues::PROVISIONING_PROFILE_TEAM_ID]

    # Use manual code signing
    automatic_code_signing(
      use_automatic_signing: false,
      path: "MyApp.xcodeproj",
      team_id: profile_team_id,
      profile_name: profile_name,
      profile_uuid: profile_name
    )

    # Build ipa
    ...

  end

But best create a new PR for this that is separate from this (You can create a new branch in your current commit, then create your further commits there - and create a PR from that. When this is merged, the first few commits will automatically be left out of your second PR or your can rebase on master)

Ok

@janpio
Copy link
Member

janpio commented Apr 15, 2019

@mollyIV That looks like what we talked about for SharedValues, correct?

@mollyIV
Copy link
Member

mollyIV commented Apr 18, 2019

That's correct @janpio 👍

According to our discussion here, SharedValues / lane_context is a preferred way of passing a value generated by one action to another (as the action's input).

Taking into account @SofteqDG comment, in this PR we would have:

module Fastlane
  module Actions
    module SharedValues
      PROVISIONING_PROFILE_NAME = : PROVISIONING_PROFILE_NAME
      PROVISIONING_PROFILE_UUID =: PROVISIONING_PROFILE_UUID
      PROVISIONING_PROFILE_TEAM_ID =: PROVISIONING_PROFILE_TEAM_ID
    end

    class InstallProvisioningProfileAction < Action
      ...
      
      def self.output
        [
          ['PROVISIONING_PROFILE_NAME', 'Description to be delivered'],
          ['PROVISIONING_PROFILE_UUID', 'Description to be delivered'],
          ['PROVISIONING_PROFILE_TEAM_ID', 'Description to be delivered']
        ]
      end
      
      ...

To get a bit more details about SharedValues and output method, please have a look at this thread 🙇

Cheers!

description: "Path to provisioning profile",
optional: false,
type: String,
verify_block: proc do |value|
Copy link
Member

Choose a reason for hiding this comment

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

What if the file path contains the spaces? Shouldn't we shellescape it? 🤔

Adding a unit test for it would be great 👍

Copy link
Member

Choose a reason for hiding this comment

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

@mollyIV No need for a shellescape here! This path is only used in Ruby stuff so it doesn't actually get pushed down to a shell command anywhere on our end 🙃

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels May 9, 2019
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 looks good! I renamed the option to :path and updated the description but this was 💯 THanks for adding this 😊

@joshdholtz joshdholtz merged commit 1abd968 into fastlane:master May 9, 2019
@fastlane-bot
Copy link

Hey @SofteqDG 👋

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 🚀

@SofteqDG SofteqDG deleted the feature/install_provisioning_profile_action branch May 10, 2019 09:51
@SofteqDG
Copy link
Contributor Author

Hi, @joshdholtz
Thanks for merging this and for your improvements!

@fastlane-bot
Copy link

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

@fastlane fastlane locked and limited conversation to collaborators Jul 15, 2019
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.

7 participants