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

Fix: Temporarily disable env var arguments for boolean default-true/exclusive flags #662

Closed
wants to merge 1 commit into from

Conversation

amcaplan
Copy link
Contributor

WHY are these changes introduced?

Apparently these never worked anyway for boolean flags before oclif 1.16.1. Then they started working, but only to specify true; otherwise, they essentially force a false as though it were specified.

More details: oclif/core#536

WHAT is this pull request doing?

Comments out the env-sourced way to set these boolean flags.

How to test your changes?

$ shopify app dev --path=fixtures/app --tunnel-url=https://my-tunnel-url:port

On main you get this error:

╭─ error ──────────────────────────────────────────────────────────────────────╮
│                                                                              │
│  The following errors occurred:                                              │
│    --tunnel-url=https://my-tunnel-url:port cannot also be provided when      │
│  using --no-tunnel                                                           │
│    --tunnel-url=https://my-tunnel-url:port cannot also be provided when      │
│  using --tunnel                                                              │
│  See more help with --help                                                   │
│                                                                              │
╰──────────────────────────────────────────────────────────────────────────────╯

On this branch, the command proceeds successfully.

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.

@amcaplan amcaplan requested review from a team as code owners October 24, 2022 08:20
@github-actions
Copy link
Contributor

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

Copy link
Contributor

@alvaro-shopify alvaro-shopify left a comment

Choose a reason for hiding this comment

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

@amcaplan
Copy link
Contributor Author

@alvaro-shopify there's little benefit to reverting that upgrade, as previously none of the env settings for boolean flags were working. (Support for that was introduced in 1.16.1... it just also happened to break these flags.) So the functionality is better with the sum total change than without it.

…ive flags

Apparently these never worked anyway for boolean flags before oclif 1.16.1.
Then they started working, but only to specify true; otherwise, they
essentially force a false as though it were specified.

More details: oclif/core#536
@amcaplan amcaplan force-pushed the workaround-boolean-flags-env-bug branch from 194cc97 to f4775a7 Compare October 24, 2022 09:41
@amcaplan amcaplan mentioned this pull request Oct 24, 2022
6 tasks
@amcaplan
Copy link
Contributor Author

Closing in favor of #666

@amcaplan amcaplan closed this Oct 25, 2022
@amcaplan amcaplan deleted the workaround-boolean-flags-env-bug branch October 25, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants