Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Indicate git is unavailable; don't error out #1823

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Dec 5, 2021

WHY are these changes introduced?

This solves a number of issues of the form of #1782

Sometimes git is necessary; other times it's just a nice-to-have. When we make API calls, we use git to check the CLI sha. While it would be nice to have, we can live without it when it's unavailable; it's not worth the DX impact. Hence, we already introduced the ShopifyCLI::Git::available? method.

However, this method doesn't check properly, as the underlying Open3::capture2e will raise an error if the underlying executable isn't available at all. (For other errors, it will actually return output and a process status as expected.) This has resulted in cryptic errors when git isn't available on the system and the user just wants to make an API call.

On my local system, intentionally making a typo to demonstrate an unavailable executable:

[2] pry(main)> Open3.capture2e('git status')
=> ["On branch git-available-fix\nnothing to commit, working tree clean\n",
 #<Process::Status: pid 69095 exit 0>]
[3] pry(main)> Open3.capture2e('jit status')
Errno::ENOENT: No such file or directory - jit
from /opt/rubies/2.7.1/lib/ruby/2.7.0/open3.rb:213:in `spawn'

WHAT is this pull request doing?

Adds a rescue for the case that the git executable isn't present. This is OK because the purpose of the available? method is only used where we don't absolutely require git.

How to test your changes?

  1. Have a system where git isn't installed
  2. Run any command (e.g. login) that makes an API call

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).

@amcaplan amcaplan requested review from a team, pepicrft, jesalerno84 and molly-yu and removed request for a team December 5, 2021 13:21
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thanks a lot for tackling this @amcaplan. I believe we'll need further work on this because git-related issues happen in Windows environments. My guess is that the way we access git only works in Unix-based systems.

@amcaplan amcaplan merged commit a6ed2e9 into main Dec 6, 2021
@amcaplan amcaplan deleted the git-available-fix branch December 6, 2021 13:40
@stephszeto
Copy link
Contributor

Thanks for the explanation here, Ariel! Could you share a demo video that shows the updated behavior? I get what this does at a high level, but am wondering what the UX actually looks like.

Also re: Pedro's comment above, is this behavior only for Linux / Mac users that don't have Git?

@amcaplan
Copy link
Contributor Author

amcaplan commented Dec 6, 2021

@stephszeto I've tested this on a Windows VM to confirm it works all around.

Here's what happens when you shopify login without git installed:

Screen Shot 2021-12-06 at 23 58 31

Following the changes in this PR, here is the results of shopify login:

Screen Shot 2021-12-07 at 0 02 50

So rather than failing because git isn't installed, we shrug it off and continue making the login web request.

I also confirmed that when git is installed on Windows, we will pick it up properly with no error, so that wasn't the cause of the error prior to this PR.

@stephszeto
Copy link
Contributor

Thanks for the screenshots!

Sometimes git is necessary; other times it's just a nice-to-have. When we make API calls, we use git to check the CLI sha. While it would be nice to have, we can live without it when it's unavailable

Do any of our CLI commands actually necessitate git to function properly, or is it a nice-to-have in all cases?

And if this solution indeed works for Windows, is there anything else we need to do for this error in general or should we have mitigated it with this PR?

@amcaplan
Copy link
Contributor Author

amcaplan commented Dec 6, 2021

Sometimes we do need git as an absolute requirement, for example when cloning an existing repository.

I believe that the linked error will be mitigated, but other errors won't be which is correct since those are the cases where we do need git.

@hannachen hannachen mentioned this pull request Dec 13, 2021
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems December 13, 2021 21:03 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants