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

[Ruby CLIKIT] - Prevent from prompting for analytics in CI #3906

Merged
merged 1 commit into from
May 17, 2024

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented May 16, 2024

WHY are these changes introduced?

WHAT is this pull request doing?

This is a patch that disables prompting for Analytics when exceptions occur in non-interactive terminal environments. This only applies to commands backed by the Ruby CLI (AFAIK the themes space is the last area we have left to migrate), so the impact surface is quite limited.

We will default to false in these environments so we don't opt-in users to send analytics without explicit consent.

How to test your changes?

  • Simulate an error raised from the Theme Access API

    • Add a raise to the first line of the create method here.
  • Run SHOPIFY_CLI_STACKTRACE=1 SHOPIFY_CLI_TTY=0 SHOPIFY_CLI_THEME_TOKEN=<your_password> SHOPIFY_FLAG_STORE=<your_shop> pnpm shopify theme push --path <path to theme> --stable -u -t "name"

The program should exit automatically rather than prompting whether you want to send analytics

Before
Screenshot 2024-05-16 at 3 22 49 PM

After - The stacktrace comes from SHOPIFY_CLI_STACKTRACE=1 and is rendered in the Before after a prompt option is selected
Screenshot 2024-05-16 at 3 21 47 PM

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/app-management

Copy link
Contributor

github-actions bot commented May 16, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
71.97% (+0.01% 🔼)
7026/9763
🟡 Branches
68.81% (+0.02% 🔼)
3479/5056
🟡 Functions 71.47% 1881/2632
🟡 Lines
73.28% (+0.01% 🔼)
6623/9038

Test suite run success

1649 tests passing in 766 suites.

Report generated by 🧪jest coverage report action from f2abb5a

@jamesmengo jamesmengo self-assigned this May 16, 2024
@jamesmengo jamesmengo added ruby Pull requests that update Ruby code Area: @shopify/cli-kit @shopify/cli-kit package issues Area: @shopify/theme @shopify/theme package issues Type: Bug Something isn't working labels May 16, 2024
@jamesmengo jamesmengo marked this pull request as ready for review May 16, 2024 22:25
Copy link
Contributor

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@jamesmengo jamesmengo requested review from karreiro, lukeh-shopify, alvaro-shopify, mgmanzella, a team and gonzaloriestra and removed request for a team May 16, 2024 22:25
Copy link
Contributor

@lukeh-shopify lukeh-shopify left a comment

Choose a reason for hiding this comment

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

🙏🏻 a review from someone with more context on this older code would be good too

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -64,6 +64,7 @@ def self.report?(context:)

def self.report_error?(context:)
return false if Environment.development?
return false unless Environment.interactive?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For readers - Defaulting to false right now to favour simplicity - given the nature of this code (living in ruby land and out of the critical path), if we see issues with this again we should consider dropping the feature altogether

@jamesmengo jamesmengo added this pull request to the merge queue May 17, 2024
Merged via the queue into main with commit c951e17 May 17, 2024
33 checks passed
@jamesmengo jamesmengo deleted the jmeng/tty_cli_fix branch May 17, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/cli-kit @shopify/cli-kit package issues Area: @shopify/theme @shopify/theme package issues ruby Pull requests that update Ruby code Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: shopify theme push shows interactive "send error report" prompt even with SHOPIFY_CLI_TTY=0
4 participants