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

Correctly read in input from a non-interactive stream for the CLI #7440

Merged
merged 1 commit into from
Oct 17, 2016

Conversation

jsternberg
Copy link
Contributor

If you pipe in a file to the influx CLI, it will not try to open the
interactive line reader, but instead just send the contents of the
entire file to the server.

Fixes #6896.

@jsternberg jsternberg force-pushed the js-6896-influx-client-from-file branch from 9c94c7c to 1924945 Compare October 10, 2016 15:57
c.Client.Addr())
}

cmd, err := ioutil.ReadAll(os.Stdin)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to read all of stdin for this? Prior to this change, stdin would have been read one line at a time, AFAICT. Should this use a bufio.Scanner instead, to handle the input line-by-line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to send the entire thing as part of the query with the current client library so I don't know if we really have a choice. Using a scanner and reading line by line won't really work since we want to send all of it as the same query and we can also split commands between multiple lines. Semi-colons are what we use as delimiters.

I do think we can possibly improve it in the future. I added support for sending the query as a file, but we don't currently use that in any of the clients.

@jsternberg jsternberg force-pushed the js-6896-influx-client-from-file branch from 1924945 to 9c15414 Compare October 10, 2016 19:43
Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jsternberg jsternberg force-pushed the js-6896-influx-client-from-file branch from 9c15414 to b1dad45 Compare October 17, 2016 16:42
If you pipe in a file to the `influx` CLI, it will not try to open the
interactive line reader, but instead just send the contents of the
entire file to the server.
@jsternberg jsternberg force-pushed the js-6896-influx-client-from-file branch from b1dad45 to 2f5f995 Compare October 17, 2016 17:58
@jsternberg jsternberg merged commit 40c75fc into master Oct 17, 2016
@jsternberg jsternberg deleted the js-6896-influx-client-from-file branch October 17, 2016 20:52
@riccardomc
Copy link

I believe I am running into an issue caused by this pull request related to the TTY check introduced in one of the commits. When executing influx CLI without a TTY, commands hang probably waiting for input from STDIN.

With the version of the CLI built from master i get the following behavior:

/inlflux-from-master/influx -database telegraf -execute 'show databases'
name: databases
name
----
telegraf
_internal
kapacitor_example

While simulating no tty allocation using ssh the command hangs indefinitely:

ssh localhost "/influx-from-master/influx -execute 'show databases'"

The same query succeeds using one of these two:

ssh -t localhost "/influx-from-master/influx -execute 'show databases'"
ssh localhost "echo 'show databases' | /influx-from-master/influx"

Notice that the query was successful using CLI 1.0.2:

$ /usr/bin/influx 
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://localhost:8086 version 1.0.2
InfluxDB shell version: 1.0.2
> exit
$ ssh localhost "/usr/bin/influx -execute 'show databases'"                    
name: databases
---------------
name
telegraf
_internal
kapacitor_example

We run into this issue when some queries executed using the latest influx CLI from a Cloud Foundry start up script for which a tty is not allocated started to fail. In that case the error was ERR: missing required parameter "q". It can reproduced by:

ssh localhost "echo '' | /influx-from-master/influx -database telegraf" 
ERR: missing required parameter "q"

This makes me believe that the influx CLI is still trying to read from STDIN in the second case.

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

4 participants