On Thu, Sep 01, 2016 at 05:17:23AM -0700, x1022as wrote:

before:

$ ./validate config-schema.json 


^C$

after:

$ ./validate config-schema.json 
Error: document.json not specified

Lots of commands (e.g. cat) block if they expect input on stdin and
stdin is still open. I don't see a problem with the current behavior.
Maybe the user intends to type the input JSON by hand or paste it in?
Or maybe they're doing some terminal magic to feed in the JSON.
And ^C is easy to do if you forget ;).

@wking thanks for reviewing.
this pr means to attach validate stdin to a pipe, the usage message takes 'cat' as an example.
I don't know about terminal magic, but I've tried like this before, and it didn't work out for me

$validate schema.json

echo '$JSON_STRING' > /proc/$VALIDATE_PID/fd/0

I'm not sure if this can make sense :)

On Thu, Sep 01, 2016 at 05:10:31PM -0700, x1022as wrote:

this pr means to attach validate stdin to a pipe, the usage message
takes 'cat' as an example.

It doesn't do anything about attaching it to a pipe, it just errors
out if it's attached to a terminal. Being attached to a terminal does
not mean ioutil.ReadAll(os.Stdin) will block forever. For example
(with a build of the master ‘validate’).

• an immediate ctrl+d raises a panic (we should probably fix this):

  $ ./validate state-schema.json
  panic: EOF

  goroutine 1 [running]:
  panic(0x70a860, 0xc820101590)
  /usr/lib/go/src/runtime/panic.go:481 +0x3e6
  main.main()
  /home/wking/src/opencontainers/runtime-spec/schema/validate.go:46 +0x5fc

• typing ‘{}’ + \n + ctrl+d:

  $ ./validate state-schema.json
  {}
  The document is not valid. see errors :

  • ociVersion: ociVersion is required
  • id: id is required
  • status: status is required
  • pid: pid is required
  • bundlePath: bundlePath is required

I haven't dug in enough to figure out how to programmatically write to
a terminal stdin and then close that stdin, but I'm sure
pseudoterminals give you a way to do that.

My point is that I don't think “special handling when stdin is a
terminal” is a good idea. You're trying to guard from:

1. User types ‘./validate state-schema.json’.
2. User sits for a long time, waiting for ‘validate’ to do something.
   Meanwhile, ‘validate’ waits watching stdin, waiting for the user
   (or someone with write-access to its stdin) to do something.

I agree that is a situation that will happen, but don't think it will
happen frequently enough to be worth guarding against. Especially
since some of the cases that guard considers errors may be valid use
cases. cat, grep, etc. all agree and treat TTYs like any other stdin.

@wking that make sense, and I think a better usage would help a lot;)

On Thu, Sep 01, 2016 at 11:13:39PM -0700, x1022as wrote:

@wking that make sense, and I think a better usage would help a lot;)

Agreed. I'd be fine with using an actual command-line-argument
library here, so you could do ‘validate --help’ and such.

@wking sorry for late response, validate seems to be very simple for now, I am not sure if we need a cli library here. cobra(as docker and runc use it) may be a little bit complex for this.
what do others think about this?

Why don't we just follow every other CLI and make it so that if you specify - as a parameter, the program takes that to mean "just read from stdin". No need for checking what the mode of stdin is or any other cleverness.

On Mon, Sep 05, 2016 at 05:39:40AM -0700, x1022as wrote:

cobra(as docker and runc use it) may be a little bit complex for
this.

I haven't seen cobra used before, but urfave/cli is pretty simple 1.

On Mon, Sep 05, 2016 at 06:19:32AM -0700, Aleksa Sarai wrote:

Why don't we just follow every other CLI and make it so that if you
specify - as a parameter, the program takes that to mean "just
read from stdin".

I think the pattern is “default to reading from stdin, unless a file
is specified whose name is not ‘-’”. For example 1:

file
A pathname of an input file. If no file operands are specified,
the standard input shall be used. If a file is '-', the cat
utility shall read from the standard input at that point in the
sequence. The cat utility shall not close and reopen standard
input when it is referenced in this way, but shall accept multiple
occurrences of '-' as a file operand.

In that case, I don't see the point of bothering with the special ‘-’.

Or are you suggesting we only support ‘-’ for stdin and error out if
no file is specified? I can't think of another command that uses that
pattern off the top of my head, but requiring an explicit argument
makes it less likely that the user accidentally invokes ‘validate’ to
read from stdin.

No need for checking what the mode of stdin is or any other
cleverness.

Agreed.

validate now works as following:

$ ./validate -s config-schema.json config.json 
The document is valid
$ cat config.json | ./validate -s config-schema.json 
The document is valid
$ ./validate -s config-schema.json 
Input your DOCUMENT CONTENT here, ended with ctrl+d or your self-defined EOF keys

ping

@vbatts you're the primary author of this script 😄 What're your thoughts on introducing urfave/cli here?

I'm not terribly opposed to this. I know urfave/cli does fancy-things-easily, but I suppose I was going to minimally functional and preferring std-lib before having remote dependencies. If you want to see that 2 arguments are required, then check flag.NArg().

side-note: i reckon we need to vendor github.com/xeipuuv/gojsonschema source code to this repo for perpetuity

ping @vbatts to make a decision :)

I'd vote for not having any dependencies in this repo. Keep it minimal.

dang @crosbymichael . I though you loved urfave/cli.

@x1022as can we do this feature without pulling in the urfave/cli repo please.

@vbatts I do love it but sometimes simple is better, I think for the needsd we have, its good to leave out in this repo.

@vbatts of course, will do this later :)

updated

This pr contains:

• add explicit usage message to validate
• schemaPath was overrided by filepath.Abs(), schemaLoader would not get the abs path.
• check local scheme and document file path with os.Stat()

LGTM