cli: various refactors #67229
Closed
cli: various refactors #67229
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
3f04d42 to
40e5aae
Compare
37b72e1 to
1eef6d6
Compare
Release note: None
Release note: None
This commit leaves the code in an intermediate stage, where the unit tests pass but maybe the linter is unhappy. This will be cleaned up in the next commit. Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
In a next step we'll want to lift configuration fields from `cliCtx` and `sqlCtx` into appropriate sub-packages. Before we can do this however, we must ensure that the fields relevant to each sub-package are in the right structs: - we want a `sqlConnCtx` which is scoped to establishing SQL client connections not just the SQL interactive shell (which has `sqlCtx`). - we want a `sqlExecCtx` which is scoped to executing and rendering SQL query results on the screen, even for non-interactive use e.g. in `cockroach node ls`. Here's a summary of the homing: | Field | Config | Note | |--------------------------------|--------------|-----------------| | `isInteractive` | `cliCtx` | unchanged | | `echo` | `sqlConnCtx` | was in `sqlCtx` | | `debugMode` | `sqlConnCtx` | was in `sqlCtx` | | `embeddedMode` | `sqlConnCtx` | was in `sqlCtx` | | `enableServerExecutionTimings` | `sqlConnCtx` | was in `sqlCtx` | | `terminalOutput` | `sqlExecCtx` | was in `cliCtx` | | `tableDisplayFormat` | `sqlExecCtx` | was in `cliCtx` | | `tableBorderMode` | `sqlExecCtx` | was in `cliCtx` | | `showTimes` | `sqlExecCtx` | was in `sqlCtx` | | `verboseTimings` | `sqlExecCtx` | was in `sqlCtx` | Additionally, this patch breaks the embedding of the `cliContext` by `sqlContext` so that the lifting into a separate package remains painless. Release note: None
This lifts the `cliCtx.isInteractive`, `sqlConnCtx` and `sqlExecCtx` into sub-packages. Release note: None
This switches the APIs from passing configs as arguments to making the functions methods of their relevant config structs. Release note: None
We'd like to move `RunQueryAndFormatResults` to a separate package and for this it needs to cease knowing about the internals of a `sqlConn`. To achieve this, this patch moves the `handleCopyError` code under control of `MakeQuery`. Since `RunQueryAndFormatResults` is always used in combination with `MakeQuery` anyway, this results in no change in functionality. Release note: None
We want to move `RunQueryAndFormatResults` to a different package and for this it needs to cease knowing about the `sqlConn` type. This commit does just that. Release note: None
Release note: None
Release note: None
This commit makes the output streams explicit in the API calls. Release note: None
This detaches the SQL shell from the internals of the demo cluster implementation. Release note: None
Release note: None
6849348 to
a6fa908
Compare
The main functionality in `demo_cluster.go` was simply moved to `democluster`. To achieve this, some untangling was needed: - the remaining `transientCluster` methods useful to the SQL shell are now all announced via an interface, so that the SQL shell doesn't need to know about the concrete type any more. - the error reporting function `checkAndShoutError()` is moved to the `clierror` package, where arguably it should have been from the start. - the `createTestCerts` function is moved to the `securitytest` package, where it belongs anyway. - the `demoLocalityList` type is moved from `cli/flags_util.go` to a new file `democluster/demo_locality_list.go` package. - the inter-region simulated latency map is moved from `cli/demo.go` to a new file `democluster/region_latencies.go`. Release note: None
This ensures there are no cobra dependencies remaining inside the interactive shell. Release note: None
"internal" tests are those that are interested in the internals of the SQL shell. The remaining tests in `sql_test.go` only use the external CLI. Release note: None
This commit ensures that the interactive shell logic only uses internal state and stops relying on global variables. Release note: None
The `EmbeddedMode` boolean is not used in the SQL shell. The two functions in `sql.go` that used it really belong to the `cliCtx` (and incidentally are currently only used from `demo.go`). This commit moves them away from `sql.go`. Release note: None
In the longer term, we will want the ability to instantiate a SQL interactive shell without the need to link in all the server code. This requires the ability to compile the SQL shell without a dependency on the `democluster` package. Towards this goal, this commit extracts the part of the `democluster.DemoCluster` interface API used in the SQL shell into a separate package. Release note: None
since it's not used there. Release note: None
This patch segregates the configuration variables that are only visible from within the interactive shell away from `sqlCtx` into a new `sqlInternalContext`. Additionally, it stops the shell from looking directly at `os.Stdout` and `stderr` and instead makes the out/err streams configurable upon invocation. Release note: None
ee3b710 to
8c7fef3
Compare
At last! The SQL shell is now independent from global state and from the CockroachDB server machinery. Release note: None
8c7fef3 to
09cb6fc
Compare
|
Superseded by #67456 and subsequent commits. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(no change in functionality)
cc @cockroachdb/server-prs