Skip to content

Conversation

@mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Mar 19, 2024

What does this PR do?

Adds preparse hook (depends on oclif/core#1005) to handle --flags-dir flag

  • --flags-dir will read files from the provided directory and insert them in the argv before the command's flags are parsed.
  • The name of the file is used as the name of the flag and the contents of the file are used as the flag's value.
  • No extension is required unless you're using json, in which case you must use .json in order for it to correctly render the json in the argv
  • Files for boolean flags must be empty (empty files will result in adding a flag without a value)
  • Files for boolean flags can be named no-<flag>, if the boolean flag supports it.
  • Files with multiple lines will be treated as multiple flags, e.g. --flag val1 --flag --val2 --flag --val3
  • Flags/values provided by the user will override any flag/values found by --flags-dir - unless the flag supports multiple, in which case they will all be combined to the final argv.
  • Files can also be named using the flag's short character.
  • No validation is done at this step so if the user has a file incorrectly named, it will still be added to the argv. oclif will throw later in the process when validating the input.
  • Hook is skipped if --flags-dir is not present
  • Hook is skipped if current command does not have --flags-dir definition

What issues does this PR fix or reference?

@W-15285609@

forcedotcom/cli#2346
forcedotcom/cli#2670
solves forcedotcom/cli#2260 (when PDR gets updated)

@mshanemc
Copy link
Contributor

QA notes:

bumped oclif/core and plugins-core in plugin-data => 3.2.1,
bumped plugin-data and plugins-core and oclif/core in the CLI

create a file called query with select id from Account
✅ ./bin/run.js data query -o hub --flags-dir ~/eng/repros/flagsDirQA

 ID                 
 ────────────────── 
 0016A00000540lTQAQ 
 0016A00000540lUQAQ 
 0016A00000540lVQAQ 
 001RQ000006EtduYAC 
Total number of records retrieved: 4.
Querying Data... done

✅ with --json gives good json results
✅ with --help gives help results , including --flags-dir

GLOBAL FLAGS
  --flags-dir=<value>  Import flag values from a directory.
  --json               Format output as json.

✅ empty boolean --use-tooling-api file

@mshanemc mshanemc merged commit 24ff247 into main Mar 22, 2024
@mshanemc mshanemc deleted the mdonnalley/flags-dir branch March 22, 2024 20:16
@Alfystar
Copy link

Fantastic 🤩 ! I'm not sure I understand what you mean here though:

No extension is required unless you're using json, in which case you must use .json in order for it to correctly render the json in the argv

Does this mean that it will be possible to create a json file in which the keys will be the flags, and the associated values will be their state?
If that were the case, it would be great 🤩 and extremely simple to create commands from wrapper programs above

@mdonnalley
Copy link
Contributor Author

Does this mean that it will be possible to create a json file in which the keys will be the flags, and the associated values will be their state?

@Alfystar Unfortunately, no - the idea here is that you can have a directory that contains files, each of which map to a flag. For instance, you could run a deploy like this:

sf project deploy start --source-dir force-app --api-version 60.0 --target-org my-scratch-org --async

Or with --flags-dir you can do this

sf project deploy start --flags-dir flags

And the flags directory looks like this

flags
- source-dir
- api-version
- target-org
- async

And each of those files contains the value for the flag (e.g. the source-dir file would contain force-app)

None of those files require extensions EXCEPT if the contents of the file are going to be JSON. In that case you need to add the .json extension so that we can correctly parse the contents.

jon-freed added a commit to jon-freed/salesforcecli-cli that referenced this pull request Jan 16, 2026
…e hook

The preparse hook was using ?? (nullish coalescing) instead of || (logical OR)
when checking if a CLI flag was already present. This caused a bug where CLI
arguments would not override flags-dir values when:

1. The flag has a short char defined (e.g., -l for --test-level)
2. The user specifies the long form on CLI (--test-level)
3. The same flag exists in flags-dir

With ??: (char && argv.includes('-l')) returns false, false ?? next returns false
With ||: (char && argv.includes('-l')) returns false, false || next evaluates next

This fix ensures CLI arguments properly override flags-dir values as documented
in PR salesforcecli#1536: "Flags/values provided by the user will override any flag/values
found by --flags-dir"

The eslint-disable is intentional because the lint rule incorrectly suggests ??
when || is actually required for proper boolean short-circuit evaluation.
jon-freed added a commit to jon-freed/salesforcecli-cli that referenced this pull request Jan 17, 2026
…e hook

The preparse hook was using ?? (nullish coalescing) instead of || (logical OR)
when checking if a CLI flag was already present. This caused a bug where CLI
arguments would not override flags-dir values in certain scenarios.

Bug introduced in: c83f81a (2025-04-22, SF CLI 2.100.0)
The change added a necessary guard (flagOptions.char &&) to prevent checking
for undefined short chars, but incorrectly used ?? instead of || to chain the
conditions. Since ?? only evaluates the right side when left is null/undefined,
and false is neither, the subsequent checks were skipped.

Affected scenarios (5 of 14 systematic test cases):
- String flag with char defined, CLI uses long form (--test-level vs -l)
- Boolean flag with char defined, CLI uses long form (--dry-run vs -d)
- Boolean allowNo flag, CLI uses --no-<flag> form to override <flag> file
- Boolean allowNo with char, CLI uses --<flag> to override no-<flag> file
- Boolean allowNo with char, CLI uses --no-<flag> to override <flag> file

Example: Flag defined as Flags.string({ char: 'l' }) with flags-dir containing
a 'test-level' file. Running `sf cmd --test-level NoTestRun` should override
the file value, but with ?? it was erroneously combined (conflict error).

This fix restores the documented behavior from PR salesforcecli#1536:
"Flags/values provided by the user will override any flag/values found by
--flags-dir -- unless the flag supports `multiple`, in which case they will
all be combined."

The eslint-disable is intentional because the lint rule incorrectly suggests ??
when || is actually required for proper boolean short-circuit evaluation.
jon-freed added a commit to jon-freed/salesforcecli-cli that referenced this pull request Jan 17, 2026
…e hook

The preparse hook was using ?? (nullish coalescing) instead of || (logical OR)
when checking if a CLI flag was already present. This caused a bug where CLI
arguments would not override flags-dir values in certain scenarios.

Bug introduced in: c83f81a (2025-04-22, SF CLI 2.100.0)
The change added a necessary guard (flagOptions.char &&) to prevent checking
for undefined short chars, but incorrectly used ?? instead of || to chain the
conditions. Since ?? only evaluates the right side when left is null/undefined,
and false is neither, the subsequent checks were skipped.

Affected scenarios (5 of 14 systematic test cases):
- String flag with char defined, CLI uses long form (--test-level vs -l)
- Boolean flag with char defined, CLI uses long form (--dry-run vs -d)
- Boolean allowNo flag, CLI uses --no-<flag> form to override <flag> file
- Boolean allowNo with char, CLI uses --<flag> to override no-<flag> file
- Boolean allowNo with char, CLI uses --no-<flag> to override <flag> file

Example: Flag defined as Flags.string({ char: 'l' }) with flags-dir containing
a 'test-level' file. Running `sf cmd --test-level NoTestRun` should override
the file value, but with ?? it was erroneously combined (conflict error).

This fix restores the documented behavior from PR salesforcecli#1536:
"Flags/values provided by the user will override any flag/values found by
--flags-dir -- unless the flag supports `multiple`, in which case they will
all be combined."

The eslint-disable is intentional because the lint rule incorrectly suggests ??
when || is actually required for proper boolean short-circuit evaluation.
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.

4 participants