-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
sql: fix license acquisition bug and add a flag to disable acquisition #46126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do a more thorough review in a 2nd pass, thanks for looking at this already
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rohany)
pkg/cli/interactive_tests/test_demo_partitioning.tcl, line 96 at r1 (raw file):
end_test start_test "Expect an error if geo-partitioning is requested and the user disabled license acquisition"
I think this could be an Example_xxx
test in cli_test.go?
please also add the -e
tests:
- license error with
-e
using a licensed feature and --disable-demo-license - no error with
-e
, licensed feature, without --disable-demo-license
0ccafca
to
83e1b64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/interactive_tests/test_demo_partitioning.tcl, line 96 at r1 (raw file):
Previously, knz (kena) wrote…
I think this could be an
Example_xxx
test in cli_test.go?please also add the
-e
tests:
- license error with
-e
using a licensed feature and --disable-demo-license- no error with
-e
, licensed feature, without --disable-demo-license
Done. I added those tests here though, because a CCL binary is required.
83e1b64
to
6590bbe
Compare
Fixes cockroachdb#46117. Release justification: bug fix Release note (bug fix): This PR fixes a bug where sometimes using `cockroach demo -e` would display a `connection refused` error. Release note (cli change): This PR adds the flag `--disable-demo-license` to provide another option to disable `cockroach demo` from attempting to acquire a demo license.
6590bbe
to
16b1930
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained
bors r=knz |
Build failed |
js oom bors r=knz |
Build succeeded |
Fixes #46117.
Release justification: bug fix
Release note (bug fix): This PR fixes a bug where sometimes using
cockroach demo -e
would display aconnection refused
error.Release note (cli change): This PR adds the flag
--disable-demo-license
to provide another option to disable
cockroach demo
from attemptingto acquire a demo license.