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

Allow flags to read its value from stdin #761

Closed
cristiand391 opened this issue Aug 15, 2023 · 13 comments · Fixed by #895
Closed

Allow flags to read its value from stdin #761

cristiand391 opened this issue Aug 15, 2023 · 13 comments · Fixed by #895

Comments

@cristiand391
Copy link
Member

cristiand391 commented Aug 15, 2023

Is your feature request related to a problem? Please describe.
Logging this request after discussing this PR with the team:
salesforcecli/plugin-auth#765

Describe the solution you'd like
Allow flags to get their value from stdin if their value is -.

This is a common pattern in CLIs, oclif supports args reading from stdin but not flags.

If input or output is a file, support - to read from stdin or write to stdout. This lets the output of another command be the input of your command and vice versa, without using a temporary file.

https://clig.dev/#arguments-and-flags

Design:
off by default, flag needs allowStdin: true and value - to read from stdin:

'sfdx-url-file': Flags.file({
      char: 'f',
      allowStdin: true,
      summary: messages.getMessage('flags.sfdx-url-file.summary'),
      required: true,
      deprecateAliases: true,
      aliases: ['sfdxurlfile'],
    })
# reads from stdin
echo $url | sf org login sfdx-url --sfdx-url-file -

# reads from file
sf org login sfdx-url --sfdx-url-file auth.json

Describe alternatives you've considered
We considered replacing --sfdx-url-file with an arg but decided this feature would:

  1. allow us to follow our style guide (prefer flags over args)
  2. benefit other CLIs

** Aditional context **

parser:
https://github.com/oclif/core/blob/main/src/parser/parse.ts
oclif also has a readStdin func to handle node 14 (still supported):

const readStdin = async (): Promise<string | null> => {

@cristiand391
Copy link
Member Author

cristiand391 commented Aug 15, 2023

From Shane:

You'd still have to have oclif prevent people from doing - on more than 1 flag.

This can be mentioned in allowStdin description but preventing this from happening in the parser is out of the scope of this request and it's up to authors to ensure only 1 flag has this enabled.

💡 : this could be prevented via eslint rules, maybe we should start doing oclif-specific ones.

@cristiand391
Copy link
Member Author

kubectl apply example:
https://kubernetes.io/docs/reference/kubectl/cheatsheet/#creating-objects

# Create multiple YAML objects from stdin
kubectl apply -f - <<EOF
apiVersion: v1
kind: Pod
...

@AllanOricil
Copy link
Contributor

AllanOricil commented Sep 24, 2023

Some commands could receive inputs from stdin without requiring developers to specify the flag that should receive the piped value.

echo 'password' | command

Example:

echo 'force://PlatformCLI::CoffeeAndBacon@su0503.my.salesforce.com' | sfdx org:login:sfdx-url

@AllanOricil
Copy link
Contributor

Is anybody working on this? If not I'm going to try

@AllanOricil
Copy link
Contributor

What is the plan to pass 2 flags values through stdin?

@AllanOricil
Copy link
Contributor

I think there should exist only one --*-stdin flag at a time. So if there are 2 flags with allowStdin, Users can't use both at the same time.

@mdonnalley
Copy link
Contributor

@AllanOricil Nobody is currently working on this so feel free to tackle it if you'd like.

I do have a few thoughts on how this should be done though.

  • Ensuring that only one flag has allowStdin set will be difficult. You could catch it at compile time, which will be difficult considering you have to consider both flags and any baseFlags defined in a base class. Alternatively, you could catch it at run time, but then the user is potentially presented with the error that the developer should have handled themselves before releasing (unlikely but it's possible).
  • Also, consider the possibility that a developer might want to have two flags that use allowStdin but are mutually exclusive so they can't ever be used at the same time. In this scenario, a compile-time solution would have to take into account both exclusive and the relationships properties. A run-time solution, however, would be able to handle this better.
  • A happy middle ground might be to emit a warning whenever multiple flags have allowStdin. You'd probably only emit this warning though if more than one of those flags are actually used.
  • Our philosophy with oclif is to provide functionality without being opinionated about what developers do with the functionality, which is quite different than how we operate on the Salesforce side of things. Because of that, my inclination is to trust developers to create good user experiences for their users. If they decide to have multiple flags with allowStdin, that's on them for creating a bad ux.

@AllanOricil
Copy link
Contributor

AllanOricil commented Nov 12, 2023

I think I found a good solution for allowing N stdin flags and to introduce stdin flags. See if you agree @mdonnalley

A flag with allowStdin forces oclif to create an additional flag called {flag-name}-stdin. The original flag works as is, but its stdin variant is the only one that can take a piped argument.

A user can specify multiple stdin flags at a time, but the stdin value that is piped to the command must have a "stdin separator", otherwise all stdin flags specified in a command receive the same value.

The stdin separator is:

  • configurable by end users
  • configurable by cli developers to all commands
  • configurable by cli developers per command

If a command has N stdin flags, but the user piped less that N values, a exception is thrown because there is no way to determine which stdin flag gets which values.

If a command has N stdin flags, and N values were passed, each value index matches the flag index. For example:

Assuming:

  • stdin separator is $
  • 2 stding flags were used in the command
  • food is a multiple flag

All the following commands would be valid:

echo "foo$bar" | bin --food-stdin --food-stdin

food-stdin[0] = foo
food-stdin[1] = bar

echo "banana" | bin --food-stdin --food-stdin

This would be invalid:

echo "a$b$c" | bin --flag-stdin --flag2-stdin
Why? Because there are 3 arg values and 2 stdin flag. If there was an arg and 2 stdin flags, this could be valid.

echo "a" | bin --flag-stdin --flag-stdin
Why? Because there is 1 arg value and 2 stdin flags.

If a flag isn't multiple and it has allowStdin, its --flag-stdin can't accept multiple arguments. The following example would throw an exception because flag-stdin isn't multiple

echo "a$b" | bin --flag-stdin --flag-stdin

stdin flags do not work with assignments. For example, both commands shown below throw an exception

bin --flag-stdin bar

bin --flag-stdin=bar

@mdonnalley
Copy link
Contributor

@AllanOricil I think that would work but I'm hesitant to recommend that approach because it's not standard CLI behavior. I think the simplest solution will be best here - even if does leave room for CLI developers to create poor user experiences.

The expected behavior here would be to have a flag that could take a value (like a string) or take -, which will tell oclif to read the value from stdin

https://clig.dev/#arguments-and-flags

If input or output is a file, support - to read from stdin or write to stdout. This lets the output of another command be the input of your command and vice versa, without using a temporary file. For example, tar can extract files from stdin:
$ curl https://example.com/something.tar.gz | tar xvf -

My vote is to have a allowStdin prop on flags that allows the flag to read the value from stdin when the flag value is -. CLI developers could misuse that property - but that's true of a lot of features in oclif.

@AllanOricil
Copy link
Contributor

AllanOricil commented Nov 13, 2023

@mdonnalley
I would really prefer not having to type - to tell my flag to use value from stdin because I have never used a cli that uses this approach, and I'm biased to how docker password-stdin flag works, which I felt it provided a really good DX. However, if you say it is the most common approach used in CLIs -, I'm fine with it. I just need to get used to.

As for allowing multiple stdin values/flags in a command feature, would it be dropped then?

  • If a user types 2 stdin flags, or 1 stding flag and 1 arg, an exception should be thrown, right?
  • Are there any other combinations that would throw an exception?

@k-capehart
Copy link
Contributor

@AllanOricil Are you still working on this? I was working on something similar but looks like this is the path forward.

@AllanOricil
Copy link
Contributor

@AllanOricil Are you still working on this? I was working on something similar but looks like this is the path forward.

Go ahead and open a PR. I did it for sfdx only and did not have time to do it for the core. I can help to review your PR and give suggestions if you need.

@k-capehart
Copy link
Contributor

@AllanOricil @mdonnalley I took a stab at a PR, feel free to pick it apart. #894

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 a pull request may close this issue.

4 participants