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

check-style: add safesql #7172

Merged
merged 1 commit into from
Jun 14, 2016
Merged

check-style: add safesql #7172

merged 1 commit into from
Jun 14, 2016

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Jun 11, 2016

Updates #2485.


This change is Reviewable

@bdarnell
Copy link
Contributor

LGTM, but why didn't this catch the very unsafe queries in cli/sql_util.go? Did we confuse it with sqlConnI?

@tamird
Copy link
Contributor Author

tamird commented Jun 12, 2016

Yep, looks that way :(

@bdarnell
Copy link
Contributor

#7139 is about to add a new unsafe query (in a test) that should be caught by safesql.

@tamird
Copy link
Contributor Author

tamird commented Jun 14, 2016

OK, I'll merge this and ask @mjibson to rebase
On Jun 13, 2016 19:54, "Ben Darnell" [email protected] wrote:

#7139 #7139 is about to
add a new unsafe query (in a test) that should be caught by safesql.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7172 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdsPO0HVW5HIZaptHI8eY7-guv8m7pIks5qLe3HgaJpZM4IzqCw
.

@tamird tamird merged commit 84c4022 into cockroachdb:master Jun 14, 2016
@tamird tamird deleted the safesql branch June 14, 2016 00:08
@maddyblue
Copy link
Contributor

maddyblue commented Jun 14, 2016

make check passes even with this test on that issue. So...?

@tamird
Copy link
Contributor Author

tamird commented Jun 14, 2016

that's weird and surprising. maybe safesql doesn't look at test code.

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.

3 participants