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

Prioritizes returning an https url over http with 'shopify tunnel status' #1221

Merged
merged 1 commit into from
May 11, 2021

Conversation

ecbrodie
Copy link
Contributor

@ecbrodie ecbrodie commented May 11, 2021

WHY are these changes introduced?

While using shopify tunnel for the first time to test out a local shopify extension over HTTPS via an ngrok tunnel, I noticed an interesting difference in the output of the following commands:

  • shopify tunnel start outputs an HTTPS URL
  • shopify tunnel status outputs an HTTP URL

IMO, to offer a more secure user experience by default, we should always be outputting an HTTPS URL instead of HTTP whenever possible.

WHAT is this pull request doing?

Before, the shopify tunnel status command would get all URLs for the ngrok tunnel and just return the first one. As of writing, ngrok returns the HTTP URL before the HTTPS one, so the HTTP URL would return first.

Now, I am using Enumerable#find to return the very first HTTPS URL. If none exists, then I fallback to existing behaviour (return the first available URL and output the same message as before).

NOTE: I have made an educated guess that this kind of change warrants a PATCH version bump. Therefore, I've incremented the version from 1.10.0 to 1.10.1. Feel free to instruct me to change my approach if necessary.

Testing

This was done from an extension I just created with shopify create extension, using the latest version of Shopify CLI downloaded from Homebrew:

➜  evan_test_localhost shopify tunnel status
Tunnel running at: http://56dfc99e384a.ngrok.io
➜  evan_test_localhost ~/src/path/to/local/shopify-app-cli/bin/shopify tunnel status
⭑ You are running a development version of the CLI at:
  /{SNIP}/shopify-app-cli/bin/shopify

Tunnel running at: https://56dfc99e384a.ngrok.io

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).

@ecbrodie ecbrodie force-pushed the tunnel-status-returns-first-https-url-if-available branch from b9dbf72 to fb18ee4 Compare May 11, 2021 16:26
@ecbrodie ecbrodie marked this pull request as ready for review May 11, 2021 16:26
@ecbrodie
Copy link
Contributor Author

Note that the tests currently failing seem unrelated to my PR. I think this project may be experiencing some CI infrastructure instability right now.

@paulomarg paulomarg requested a review from a team May 11, 2021 16:41
lib/shopify-cli/version.rb Outdated Show resolved Hide resolved
@ecbrodie ecbrodie force-pushed the tunnel-status-returns-first-https-url-if-available branch from fb18ee4 to 5c036fa Compare May 11, 2021 17:43
@ecbrodie ecbrodie force-pushed the tunnel-status-returns-first-https-url-if-available branch from 5c036fa to 85a6b8a Compare May 11, 2021 17:43
@ecbrodie ecbrodie merged commit a6ad019 into master May 11, 2021
@ecbrodie ecbrodie deleted the tunnel-status-returns-first-https-url-if-available branch May 11, 2021 19:18
@ecbrodie
Copy link
Contributor Author

Thanks for the review :)

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.

None yet

3 participants