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

USE databasename will work only for existing databases. #5183

Merged
merged 2 commits into from
Dec 22, 2015
Merged

USE databasename will work only for existing databases. #5183

merged 2 commits into from
Dec 22, 2015

Conversation

pires
Copy link
Contributor

@pires pires commented Dec 21, 2015

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA (if not already signed)

Fixes #5174

Unfortunately, I don't see a way of testing this without implementing some database behavior on cli_test.go, namely to return one existing database.

$ influx -host 192.168.99.100
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://192.168.99.100:8086 version 0.9.6.1
InfluxDB shell 0.9
> show databases
name: databases
---------------
name
_internal

> use pires
ERR: Database pires doesn't exist. Run SHOW DATABASES for a list of existing databases.
> use _internal
Using database _internal

@pires
Copy link
Contributor Author

pires commented Dec 21, 2015

Also, I'd like to know why name shows up as a database when SHOW DATABASES runs. It sounds like a bug when formatting the response to output but I want this PR to be validated before looking into that issue - I'll open a new one though.

@beckettsean
Copy link
Contributor

@pires name is not a database name, it's because the query response format is a recycled structure. If you do any SELECT or SHOW query, the first line of results are the column names. In this case, there's only one column returned, the list of database names, and the column title is name.

@pires
Copy link
Contributor Author

pires commented Dec 21, 2015

@beckettsean indeed, thank you. Does this LGTY?

@beckettsean
Copy link
Contributor

@pires again I can't qualify Go code, but it looks like the right approach. It might be a little weird to get error messages for a SHOW DATABASES query when I typed USE foo into the CLI. Perhaps the error code could prepend an explanation to the error message to make that clearer?

response, err := c.Client.Query(client.Query{Command: query})
    if err != nil {
        fmt.Printf("ERR:  Cannot fetch list of databases: %s\n", err)
        return
    }

if err := response.Error(); err != nil {
        fmt.Printf("ERR: Cannot fetch list of databases: %s\n", err)
        return
    }

fmt.Printf("Using database %s\n", d)

// validate if specified database exists
query := "SHOW DATABASES"
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor, but just shove SHOW DATABASES inside the call to Query. A new variable isn't really needed, don't you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@otoolep
Copy link
Contributor

otoolep commented Dec 22, 2015

Works for me, some minor improvements suggested. +1

@corylanou

@corylanou
Copy link
Contributor

+1.

@pires
Copy link
Contributor Author

pires commented Dec 22, 2015

@beckettsean you may find issues with the database that are not related to the database not existing. Said verifications follow the same pattern as other queries. SHOW DATABASES is a query.

@pires
Copy link
Contributor Author

pires commented Dec 22, 2015

@corylanou @otoolep comments addressed. Thank you!

otoolep added a commit that referenced this pull request Dec 22, 2015
USE databasename will work only for existing databases.
@otoolep otoolep merged commit ff20ec6 into influxdata:master Dec 22, 2015
@pires pires deleted the 5174-fix_use_db branch December 22, 2015 14:38
jsternberg added a commit that referenced this pull request Dec 23, 2015
PR #5183 added a validation for `use` to only be able to select public
databases so `_internal` couldn't be chosen. To implement this, the
`SHOW DATABASES` command was used by the internal client.

Some of the unit tests in `cli_test.go` don't set the client to
anything. `TestParseCommand_Use` previous didn't, but now it needs to
have a client in the unit test with an empty test server.
jsternberg added a commit that referenced this pull request Dec 29, 2015
One of the first unit tests in the cli tests called the Run method.
Since the Run method called os.Exit, it reported the unit tests as
succeeded. When parallel is set to 1, this skips _all_ unit tests after
the first one. When parallel is set to a higher value, unit tests run by
other processes still get run.

This changes the Run method to return an error (if one occurred). This
error can then be printed out and a bad exit status can be used to exit
the program from the main program instead.  That causes the unit tests
to run correctly regardless of how many parallel processes are running.

Also added an additional option to the CLI called `IgnoreSignals`. If
this is set to true, then signals are not registered with the process.
Setting signals doesn't really work in unit tests so it's good to ensure
they don't get set in the first place.

In addition to fixing the influx cli tests, this adds a mock client to
the cli test for Use. PR #5183 added a validation for `use` to only be
able to select public databases so `_internal` couldn't be chosen. To
implement this, the `SHOW DATABASES` command was used by the internal
client.

Some of the unit tests in `cli_test.go` don't set the client to
anything. `TestParseCommand_Use` previously didn't, but now it needs to
have a client in the unit test with an empty test server.
@jwilder jwilder added this to the 0.10.0 milestone Feb 1, 2016
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.

5 participants