[spaceauth] add `--copy_to_clipboard` option to `spaceauth` #18538

rogerluan · 2021-04-10T23:14:22Z

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

Resolves #17843

Description

After investigating this issue, I tried to accomplish what both users in #17843 were seeking (eval $(fastlane spaceauth --eval)). However, that wasn't possible exactly as is because it would require fastlane to not print anything else to stdout except the export command, and fastlane doesn't have a global --silent argument or anything similar - it may print out session-related logs not directly related to spaceauth, or even the initial [✔] 🚀.
To work around this issue, the solution I came up with was to export the session to the clipboard without prompting the user for it, specially in a CI environment.

Documentation

I've documented this new option in spaceship/README.md (in this PR) and also here fastlane/docs#1049

Testing Steps

To test this branch, modify your Gemfile as:

gem "fastlane", :git => "https://github.com/fastlane/fastlane.git", :branch => "origin/rogerluan/spaceauth-export-to-clipboard"

And run bundle install to apply the changes.

Then run bundle exec fastlane spaceauth -u user@example.org --exports_to_clipboard && eval $(pbpaste) and echo $FASTLANE_SESSION to see if the session was correctly exported.

cc @return-main @Semmu

spaceship/README.md

MarcPerezPro · 2021-04-11T12:13:23Z

spaceship/lib/spaceship/spaceauth_runner.rb


-      if mac? && Spaceship::Client::UserInterface.interactive? && agree("🙄 Should fastlane copy the cookie into your clipboard, so you can easily paste it? (y/n)", true)
+      if @exports_to_clipboard


I like that you are not checking for the platform here, that way we can set:

alias pbcopy='xsel --clipboard --input' alias pbpaste='xsel --clipboard --output'

OR

alias pbcopy='xclip -selection clipboard' alias pbpaste='xclip -selection clipboard -o'

for use on Linux machines.

Very interesting point @return-main ! Once I saw the failing specs I thought of either one of the two:
a) Make it work on all OS;
b) Make it work only on macOS.

The first would be a bit tricky. I could add a new dependency such as https://github.com/janlelis/clipboard or implement it on my own, but it'd likely be wonky.
Making it work only on macOS would be kinda limited. Once I saw this I refactored my code so that it should work on linux if you have an alias set up.

Thanks! 🙌

MarcPerezPro

Looks good to me, and it's useful for our use-case.

  desc 'Updates the FASTLANE_SESSION in AWS Secrets Manager.'
  desc 'We will have to run this lane once a month.'
  desc "It's interactive, so no running on Bitrise."
  desc "You'll need to input the SMS code manually."
  lane :update_fastlane_session do
    fastlane_session_key = 'FASTLANE_SESSION'
    if ENV.has_key?(fastlane_session_key)
      fastlane_session = ENV[fastlane_session_key]
      UI.important "Loaded #{fastlane_session_key} from Envrionment"
    else
      Spaceship::SpaceauthRunner.new(exports_to_clipboard: true).run
      fastlane_session = `pbpaste`.match(/export FASTLANE_SESSION=\'(.*)\'/).captures[0]
    end

    client = Aws::SecretsManager::Client.new()
    resp = client.get_secret_value({
      secret_id: "arn:aws:secretsmanager:████:████:secret:IOSEnvironment"
    })
    secrets = JSON.parse(resp.secret_string)
    secrets[fastlane_session_key] = fastlane_session
    client.put_secret_value({
      secret_id: resp.arn,
      secret_string:  JSON.generate(secrets)
    })
    UI.important "Successfully set the #{fastlane_session_key} in AWS Secrets Manager."
  end

rogerluan · 2021-04-11T19:08:25Z

@return-main thank you for your review!

I've addressed your points and would be happy if you could review this PR again 🙏
I have a design question though:

fastlane_session = pbpaste.match(/export FASTLANE_SESSION='(.*)'/).captures[0]

Do you think it would make more sense to have the export_to_clipboard option export only the session escaped string (i.e. removing export FASTLANE_SESSION= prefix), or perhaps it's good as is? Which one you think it's gonna be more flexible, or attend to users' expectations more often? 😁 also @Semmu maybe you could share how you're using this as well 😇

MarcPerezPro · 2021-04-11T19:50:12Z

@return-main thank you for your review!

I've addressed your points and would be happy if you could review this PR again 🙏
I have a design question though:

fastlane_session = pbpaste.match(/export FASTLANE_SESSION='(.*)'/).captures[0]

Do you think it would make more sense to have the export_to_clipboard option export only the session escaped string (i.e. removing export FASTLANE_SESSION= prefix), or perhaps it's good as is? Which one you think it's gonna be more flexible, or attend to users' expectations more often? 😁 also @Semmu maybe you could share how you're using this as well 😇

For our use-case, separating the cookie filtering logic into a function would be perfect. It would allow us to programmatically get the FASTLANE_SESSION without any side-effects on the clipboard. If that's too complicated, then just returning the FASTLANE_SESSION without prefix at the end should be enough.

For export_to_clipboard, I think that keeping the export FASTLANE_SESSION= prefix should be easier for the majority of Fastlane users. But at the same time, this behaviour differs from the interactive clipboard copying logic that was previously in place:

if mac? && Spaceship::Client::UserInterface.interactive? && agree("🙄 Should fastlane copy the cookie into your clipboard, so you can easily paste it? (y/n)", true)

Semmu · 2021-04-11T20:21:08Z

Hey @rogerluan ,

Thanks for picking up this task! It will be certainly useful for us in my company!

I have read through the code and the discussion here and these are my thoughts:

Some kind of --silent or an equivalent would be the best, where only the export ...=... line (or even better, just the session ID) is printed - but as you mentioned, it would required a huge refactoring and would affect most of the codebase, so it is definitely out of scope.
The clipboard as a workaround seems good, but now as you started writing the core utility for that, it would be the best if fastlane would support all/most platforms, not only macOS. (Though I'm not sure about fastlane usage on other platforms 🤷 )
Even though with this approach, I would only copy the resulting session ID to the clipboard (omitting the export keyword and such), so people can use the session ID whatever way they want without cluttering the environment, for example inline, such as:
```
FASTLANE_SESSION=$(pbpaste) fastlane ...
```

What do you think?

MarcPerezPro

LGTM

MarcPerezPro · 2021-04-11T20:26:54Z

fastlane_core/lib/fastlane_core.rb

+require_relative 'fastlane_core/test_parser'
+require_relative 'fastlane_core/ui/errors'
+require_relative 'fastlane_core/ui/ui'
+require_relative 'fastlane_core/update_checker/update_checker'


I like reorganising dependencies alphabetically, but it may have some unintended side-effects in Ruby.
This PR works on my machine, but let's wait for CircleCI's unit tests to complete before merging.

As long as the tests pass, is this safe? I have no clue 😮 what are/were the risks? maybe cc @ainame 🙏 😁

CircleCI passed, so it should be safe ✅

As you saw in fastlane_core.rb, some requires have to be ordered before:

# Ruby monkey-patches - should be before almost all else require_relative 'fastlane_core/core_ext/string' require_relative 'fastlane_core/core_ext/shellwords'

And some after:

require 'commander' # after commander import require_relative 'fastlane_core/ui/fastlane_runner' # monkey patch

They are usually extensions of Ruby classes, without those we'll probably get NoMethodErrors in CircleCI.

As long as test cases cover those files, it would be fine🙂 Even in the monkey patch scenario mentioned, it doesn't matter as long as Ruby doesn't evaluate such patched methods directly.

For example, let's say we need to load two files in fastlane_core.rb and file A is like below, it is safe. The order doesn't matter. monkey_patch_method_return_true will be evaluated after everything loaded.

# fastlane_core.rb require 'file A' require 'monkey_patch'

# file A' class A def hello puts("hello") if "some_string".monkey_patch_method_return_true end end

However, file A' causes an error because we need to require file 'moneky_patch' that implements monkey_patch_method_return_true to String before file A is loaded.

# file A if "some_string".monkey_patch_method_return_true class A def hello; end end end

commander scenario surely depends on the gem to define the classes first and then monkey patching follows.

You can test whether that file loading works by ruby -Ifastlane_core/lib fasltane_core/lib/fastlane_core.rb. Even so, there is a small chance order updates create logic-level discrepancy but unit tests should cover that 🤞

It's off-topic but fastlane's unit testing runs test cases exactly the same order every time right now so combining existing require order and unit test cases order may create a happy code path that still leaves some edge-cases technically😬

I've been trying to improve that in a PR...
#18278

This slightly terrifies me but I do prefer it this way 😬 I don’t think this should causes any issues but still a bit spooky 🤷‍♂️

rogerluan · 2021-04-12T12:43:04Z

@return-main yeah the inconsistency bothered me when implementing, but I designed it to be as close to what you guys were looking forward to use 😄but I think we can achieve both results, with just a little bit more typing on either end, depending on which way we go.

@Semmu I think the current state for cross-platform support for this feature is decent. We already have a clipboard action that never had built-in support for Linux or Windows, so I think it's fine as is. I wouldn't want to re-implement the Clipboard gem, but we might want to decide to add it into fastlane in the future (separate from this PR) if we see necessity - in general we avoid adding more and more dependencies to fastlane unless it's really needed. So there is a significant amount of users facing problems, we can reconsider 😊

Guys, I addressed your latest comments! I changed the clipboard content to contain only the cookie, and updated the docs (here), those were all great and juicy feedbacks! Thanks all!

Lemme know your thoughts on this latest iteration 😊 The only thing I would reconsider is perhaps the name of the option exports_to_clipboard - I was thinking copy_to_clipboard, thoughts?

MarcPerezPro · 2021-04-12T13:16:51Z

spaceship/lib/spaceship/spaceauth_runner.rb

      end
+
+      return self


Thank you for that, it simplified my logic to:

fastlane_session = Spaceship::SpaceauthRunner.new().run.instance_variable_get(:@yaml)

Oh I just noticed the session_string method.
So the proper way is:

fastlane_session = Spaceship::SpaceauthRunner.new().run.session_string

MarcPerezPro

LGTM 👍

Semmu · 2021-04-12T13:40:07Z

Hey @rogerluan,

The latest changes look really nice! I agree with your point on clipboard support, this solution is sufficient enough.

And also I agree on your naming concerns, maybe copy_to_clipboard would make more sense now with the latest changes 😄

joshdholtz

Small suggestions to rename to copy_to_clipboard 😬

joshdholtz · 2021-04-14T02:24:50Z

fastlane_core/lib/fastlane_core.rb

+require_relative 'fastlane_core/test_parser'
+require_relative 'fastlane_core/ui/errors'
+require_relative 'fastlane_core/ui/ui'
+require_relative 'fastlane_core/update_checker/update_checker'


This slightly terrifies me but I do prefer it this way 😬 I don’t think this should causes any issues but still a bit spooky 🤷‍♂️

spaceship/lib/spaceship/commands_generator.rb

ainame · 2021-04-13T07:31:49Z

fastlane/lib/fastlane/actions/clipboard.rb

      end

      def self.is_supported?(platform)
-        true
+        return FastlaneCore::Clipboard.is_supported?


Suggested change

return FastlaneCore::Clipboard.is_supported?

FastlaneCore::Clipboard.is_supported?

Should this have been caught by linter? Or should we configure it to do so? 🤔
I've changed this specific instance 👍 but what do you think?

There is a rule for this but the current Rubocop version fastlane uses don't support it 😢
https://docs.rubocop.org/rubocop/cops_style.html#styleredundantretur

(I just started working on the bumping version lately but there's a lot of to do for it😂 )

ahh got it!
Yeah I think we should really work on bumping its version then 😮 but bumping linter version (if there's no autocorrect) is soooo troublesome 🙈
Lmk if there's an open PR or branch I can contribute to!

fastlane_core/lib/fastlane_core.rb

spaceship/lib/spaceship/commands_generator.rb

rogerluan · 2021-04-14T19:36:47Z

Addressed comments and updated fastlane/docs#1049 once again 😊

spaceship/lib/spaceship/spaceauth_runner.rb

joshdholtz

Few more small questions/suggestions 😇 Sorry!

joshdholtz · 2021-04-19T10:35:48Z

spaceship/lib/spaceship/spaceauth_runner.rb


 require_relative 'tunes/tunes_client'

 module Spaceship
  class SpaceauthRunner
-    def initialize(username: nil)
+    def initialize(username: nil, copy_to_clipboard: nil)


Suggested change

def initialize(username: nil, copy_to_clipboard: nil)

def initialize(username: nil, copy_to_clipboard: false)

That would change the default behaviour...
See #18538 (comment)

☝️ Yeah, here the absence of the param behaves differently than an explicit false value, hence why the check up there is == false instead of a 'falsey' check

spaceship/README.md

joshdholtz · 2021-04-19T10:40:34Z

spaceship/lib/spaceship/spaceauth_runner.rb

      end
+
+      return self


Can this just return @yaml? 😇 This is already running on an instance so I don’t think returning self is as useful as returning the actual value? 🤔

This also gets rid of the need for the session_string addition below 🤷‍♂️

cc: @rogerluan @return-main

I'd be okay with that, but by returning self we can future-proof the lane in case we create new methods.

I share the same thoughts as @return-main ☝️
Not sure how/if this class plans to be changed much in the future, but it's a possibility. It would make it possible to create new methods similar to how session_string works, without worrying about retrocompatibility.

What do you think @joshdholtz ?

I’m fine with that 😇 We should be good to go then

joshdholtz

This is 🔥 Thanks so much for adding this and handling all the reviews ❤️ You my hero!

fastlane-bot · 2021-04-20T19:30:54Z

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

rogerluan added 2 commits April 10, 2021 20:08

Add --exports_to_clipboard option to spaceauth.

f5d9930

Add documentation for exports_to_clipboard option.

ae54760

rogerluan added the tool: spaceship label Apr 10, 2021

google-cla bot added the cla: yes label Apr 10, 2021

rogerluan added the type: feature label Apr 10, 2021

rogerluan mentioned this pull request Apr 10, 2021

Document spaceauth's --copy_to_clipboard option fastlane/docs#1049

Merged

MarcPerezPro reviewed Apr 11, 2021

View reviewed changes

spaceship/README.md Outdated Show resolved Hide resolved

MarcPerezPro reviewed Apr 11, 2021

View reviewed changes

MarcPerezPro approved these changes Apr 11, 2021

View reviewed changes

rogerluan added 4 commits April 11, 2021 14:26

Add FastlaneCore::Clipboard utility.

42c0dc0

Fix clipboard action platform availability.

4b14cef

Make clipboard be more resilient on unsupported OS.

aef508d

Document exports_to_clipboard OS limitations.

b2b214f

Add missing require.

07acc41

MarcPerezPro approved these changes Apr 11, 2021

View reviewed changes

rogerluan added 4 commits April 12, 2021 09:09

Export only the session itself.

bbf9c33

Add #session_string method to SpaceauthRunner.

203c687

Fix "Application-specific passwords" external link.

20f3d8a

Redirect official docs instead of duplicating it.

88ce490

MarcPerezPro reviewed Apr 12, 2021

View reviewed changes

MarcPerezPro approved these changes Apr 12, 2021

View reviewed changes

joshdholtz requested changes Apr 14, 2021

View reviewed changes

ainame reviewed Apr 14, 2021

View reviewed changes

Rename option from exports_to_clipboard to copy_to_clipboard.

829064f

rogerluan added 2 commits April 14, 2021 16:31

Code style.

0934348

Merge branch 'master' into rogerluan/spaceauth-export-to-clipboard

cf68c18

rogerluan requested a review from joshdholtz April 14, 2021 19:34

rogerluan changed the title ~~[spaceauth] add --exports_to_clipboard option to spaceauth~~ [spaceauth] add --copy_to_clipboard option to spaceauth Apr 14, 2021

MarcPerezPro reviewed Apr 16, 2021

View reviewed changes

spaceship/lib/spaceship/spaceauth_runner.rb Outdated Show resolved Hide resolved

Allow non-interactive script in interactive session.

7ded3b6

joshdholtz requested changes Apr 19, 2021

View reviewed changes

MarcPerezPro approved these changes Apr 19, 2021

View reviewed changes

Update docs to point to new docs page introduced in fastlane/docs#1052

6572f48

rogerluan requested a review from joshdholtz April 19, 2021 13:29

joshdholtz approved these changes Apr 20, 2021

View reviewed changes

joshdholtz merged commit 0caaadf into master Apr 20, 2021

joshdholtz mentioned this pull request Apr 20, 2021

Version bump to 2.181.0 #18578

Merged

rogerluan deleted the rogerluan/spaceauth-export-to-clipboard branch April 20, 2021 11:53

fastlane-bot added the status: released label Apr 20, 2021

WangLongONE mentioned this pull request Apr 28, 2021

fastlane update_fastlane not used, Updating fastlane from 2.180.1 to 2.181.0... #18616

Closed

fastlane locked and limited conversation to collaborators Jun 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spaceauth] add `--copy_to_clipboard` option to `spaceauth` #18538

[spaceauth] add `--copy_to_clipboard` option to `spaceauth` #18538

rogerluan commented Apr 10, 2021 •

edited

MarcPerezPro Apr 11, 2021

rogerluan Apr 11, 2021

MarcPerezPro left a comment •

edited

rogerluan commented Apr 11, 2021 •

edited

MarcPerezPro commented Apr 11, 2021

Semmu commented Apr 11, 2021

MarcPerezPro left a comment

MarcPerezPro Apr 11, 2021

rogerluan Apr 12, 2021 •

edited

MarcPerezPro Apr 12, 2021

ainame Apr 13, 2021 •

edited

joshdholtz Apr 14, 2021

rogerluan commented Apr 12, 2021

MarcPerezPro Apr 12, 2021

MarcPerezPro Apr 12, 2021

MarcPerezPro left a comment

Semmu commented Apr 12, 2021

joshdholtz left a comment

joshdholtz Apr 14, 2021

ainame Apr 13, 2021

rogerluan Apr 14, 2021

ainame Apr 15, 2021 •

edited

rogerluan Apr 16, 2021 •

edited

rogerluan commented Apr 14, 2021

joshdholtz left a comment

joshdholtz Apr 19, 2021

MarcPerezPro Apr 19, 2021 •

edited

rogerluan Apr 19, 2021 •

edited

joshdholtz Apr 19, 2021

MarcPerezPro Apr 19, 2021

rogerluan Apr 19, 2021 •

edited

joshdholtz Apr 20, 2021

joshdholtz left a comment

fastlane-bot commented Apr 20, 2021


		if mac? && Spaceship::Client::UserInterface.interactive? && agree("🙄 Should fastlane copy the cookie into your clipboard, so you can easily paste it? (y/n)", true)
		if @exports_to_clipboard

	return FastlaneCore::Clipboard.is_supported?
	FastlaneCore::Clipboard.is_supported?

	def initialize(username: nil, copy_to_clipboard: nil)
	def initialize(username: nil, copy_to_clipboard: false)

[spaceauth] add --copy_to_clipboard option to spaceauth #18538

[spaceauth] add --copy_to_clipboard option to spaceauth #18538

Conversation

rogerluan commented Apr 10, 2021 • edited

Checklist

Motivation and Context

Description

Documentation

Testing Steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcPerezPro left a comment • edited

Choose a reason for hiding this comment

rogerluan commented Apr 11, 2021 • edited

MarcPerezPro commented Apr 11, 2021

Semmu commented Apr 11, 2021

MarcPerezPro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ainame Apr 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcPerezPro left a comment

Choose a reason for hiding this comment

Semmu commented Apr 12, 2021

joshdholtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ainame Apr 15, 2021 • edited

Choose a reason for hiding this comment

rogerluan Apr 16, 2021 • edited

Choose a reason for hiding this comment

rogerluan commented Apr 14, 2021

joshdholtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcPerezPro Apr 19, 2021 • edited

Choose a reason for hiding this comment

rogerluan Apr 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan Apr 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdholtz left a comment

Choose a reason for hiding this comment

fastlane-bot commented Apr 20, 2021

[spaceauth] add `--copy_to_clipboard` option to `spaceauth` #18538

[spaceauth] add `--copy_to_clipboard` option to `spaceauth` #18538

rogerluan commented Apr 10, 2021 •

edited

MarcPerezPro left a comment •

edited

rogerluan commented Apr 11, 2021 •

edited

rogerluan Apr 12, 2021 •

edited

ainame Apr 13, 2021 •

edited

ainame Apr 15, 2021 •

edited

rogerluan Apr 16, 2021 •

edited

MarcPerezPro Apr 19, 2021 •

edited

rogerluan Apr 19, 2021 •

edited

rogerluan Apr 19, 2021 •

edited