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] Generate update_code_signing_settings action instead of deprecated automatic_code_signing #15900

Merged
merged 11 commits into from Mar 23, 2020
Merged

Conversation

att55
Copy link
Contributor

@att55 att55 commented Jan 15, 2020

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

automatic_code_signing has use_automatic_signing, the key switches Auto to Manual code signing. And the defaulut value is false.

https://docs.fastlane.tools/actions/automatic_code_signing/#automatic_code_signing

Sample code is as below.

automatic_code_signing(
  path: "demo-project/demo/demo.xcodeproj",
  use_automatic_signing: false
)

automatic_code_signing(
  path: "demo-project/demo/demo.xcodeproj",
  use_automatic_signing: true
)

But, I feel the specifications below are strange and confusing users.

  1. Even though the action name is automatic_code_signing, the action allows to switch Auto to Manual code signing.
  2. Even though the action name is automatic_code_signing, Manual code signing is executed by default because the default value of use_automatic_signing is false.

So, I added code_signing as alias of automatic_code_signing to prevent confusing users.

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.

@att55 👋 This is a very interesting change! I'm a little bit for but also a little bit against this 🤔 I do agree that this action should have a different name but I also don't want to just introduce another action/alias.

Aliases are nice (in this cases since some of the options conflict with the name of the action) but it makes things a little bit harder to maintain and harder to find other actions.

If we are going to do this, I would rather move the logic from AutomaticCodeSigning into one that is better named and add a message saying that we are deprecating AutomaticCodeSigning in the future.

The code_signing name feels a bit too generic to me (I actually thought this PR was for something like sigh or cert at first). I think it would need to be something like update_code_signing_settings

Thoughts?

@att55
Copy link
Contributor Author

att55 commented Jan 28, 2020

@joshdholtz Thank you so much for your review!

I would rather move the logic from AutomaticCodeSigning into one that is better named and add a message saying that we are deprecating AutomaticCodeSigning in the future.
I think it would need to be something like update_code_signing_settings

I think your plan is very good and update_code_signing_settings is effective to resolve this issue! 😸
I will try to adopt your plan in this PR 👍

@att55
Copy link
Contributor Author

att55 commented Jan 30, 2020

@joshdholtz I adopted our plan in this PR 👍Please check when you can 🙏

The outline is below.

  • added the message saying that we are deprecating automatic_code_signing in automatic_code_signing action .
  • generated update_code_signing_setttings action instead of automatic_code_signing
  • added a test file for update_code_signing_setttings
  • added fixtures(xcodeproj) to exec the test of update_code_signing_setttings

@att55 att55 changed the title [automatic_code_signing] Add code_signing as alias [action] Generate update_code_signing_settings action instead of deprecated automatic_code_signing Feb 5, 2020
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.

Thanks SO much for making the changes I suggested 🥰 One more small change! If we could use UI.deprecated instead of UI.important that would be 💯

fastlane/lib/fastlane/actions/automatic_code_signing.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/automatic_code_signing.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
fastlane/spec/actions_specs/automatic_code_signing_spec.rb Outdated Show resolved Hide resolved
Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>

use ui.deprecated

Co-Authored-By: Josh Holtz <josh@rokkincat.com>
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@att55
Copy link
Contributor Author

att55 commented Feb 19, 2020

@googlebot I consent.

@att55
Copy link
Contributor Author

att55 commented Feb 19, 2020

I thought I've already consented CLAs... 🤔??

@att55
Copy link
Contributor Author

att55 commented Feb 19, 2020

@joshdholtz Thank you so much for your review and requesting suggested changes to this PR!
I fixed to use UI.deprecated instead of UI.important. 👍
Sorry to bother you, but please check when you can 😄

@att55
Copy link
Contributor Author

att55 commented Feb 19, 2020

Sorry... I didn't understand how to pass "size-label" and "cla/google"...

@att55
Copy link
Contributor Author

att55 commented Mar 9, 2020

@joshdholtz Sorry to bother you... please check this PR when you can 🙏

@joshdholtz
Copy link
Member

Yo yo yo! Will check first thing tomorrow morning ❤️

@att55
Copy link
Contributor Author

att55 commented Mar 11, 2020

Thank you so much ❤️ It's file, when you really have time 😸

@att55
Copy link
Contributor Author

att55 commented Mar 23, 2020

@joshdholtz ( When you can... 👀🙏 )

@joshdholtz
Copy link
Member

Aye aye! Getting back to things first thing tomorrow morning 👌

@joshdholtz
Copy link
Member

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

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.

Looks good to me! Thanks for making those changes 😊 Appreciate all your contributions ❤️

@joshdholtz joshdholtz merged commit 63be9d3 into fastlane:master Mar 23, 2020
@fastlane-bot
Copy link

Hey @att55 👋

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

@fastlane fastlane locked and limited conversation to collaborators May 23, 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

5 participants