Fix streaming of stdout, stdin, stderr in cobra test runner by shreyas-goenka · Pull Request #1742 · databricks/cli

shreyas-goenka · 2024-09-02T11:57:18Z

Changes

We were not using the readers and writers set in the test fixtures in the progress logger. This PR fixes that. It also modifies TestAccAbortBind, which was implicitly relying on the bug.

I encountered this bug while working on #1672.

Tests

Manually.

From non-tty:

Error: failed to bind the resource, err: This bind operation requires user confirmation, but the current console does not support prompting. Please specify --auto-approve if you would like to skip prompts and proceed.

From tty, bind works as expected.

Confirm import changes? Changes will be remotely applied only after running 'bundle deploy'. [y/n]: y
Updating deployment state...
Successfully bound databricks_pipeline with an id '9d2dedbb-f522-4503-96ba-4bc4d5bfa77d'. Run 'bundle deploy' to deploy changes to your workspace

shreyas-goenka · 2024-09-02T12:24:43Z

Triggered the nightlies on this...

pietern

The downside of the black-box runner is that you can no longer step through CLI code when debugging an issue. I'd like to avoid using it unless absolutely necessary, like it was in the case where a library was writing to stdout/stderr.

shreyas-goenka

I forgot to post these comments when asking for a review. Doing so now.

shreyas-goenka · 2024-09-02T11:58:01Z

 func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) {
+	// No need to initialize the logger if it's already set in the context. This
+	// happens in unit tests where the logger is setup as a fixture.
+	if _, ok := cmdio.FromContext(ctx); ok {


Before, we would not use the stdin/out/err io readers/writers in internal.NewCobraTestRunner(). This passes them through now.

shreyas-goenka · 2024-09-02T12:00:02Z

-_, _, err = c.Run()
-	require.ErrorContains(t, err, "failed to bind the resource")
+// Bind should fail because prompting is not possible.
+stdout, stderr, err := blackBoxRun(t, bundleRoot, "bundle", "deployment", "bind", "foo", fmt.Sprint(jobId))


The original test fails because it implicitly relied on the stdin not being passed to the cobra test runner, which caused an error here because of EOF on stdin.
The test is now improved, and the error message is more prescriptive to use the --auto-approve flag. Testing the prompting library itself is out of scope for this test.

shreyas-goenka · 2024-09-02T13:18:59Z

Nightlies are green and happy with this PR.

CLI: * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)). Bundles: * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)). * Add prompt when a pipeline recreation happens ([#1672](#1672)). * Use materialized views in the default-sql template ([#1709](#1709)). * Update templates to latest LTS DBR ([#1715](#1715)). * Make lock optional in the JSON schema ([#1738](#1738)). * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)). * Include a permissions section in all templates ([#1713](#1713)). * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)). * Fixed variable override in target with full variable syntax ([#1749](#1749)). Internal: * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)). * PythonMutator: explain missing package error ([#1736](#1736)). * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)). * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)). Dependency updates: * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)).

CLI: * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)). Bundles: As of this release CLI will show a prompt is if there are configuration changes which will lead to a DLT recreation. Users can skip the prompt by specifying the `--auto-approve` flag * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)). * Add prompt when a pipeline recreation happens ([#1672](#1672)). * Use materialized views in the default-sql template ([#1709](#1709)). * Update templates to latest LTS DBR ([#1715](#1715)). * Make lock optional in the JSON schema ([#1738](#1738)). * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)). * Include a permissions section in all templates ([#1713](#1713)). * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)). * Fixed variable override in target with full variable syntax ([#1749](#1749)). Internal: * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)). * PythonMutator: explain missing package error ([#1736](#1736)). * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)). * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)). Dependency updates: * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)). --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

## Changes We were not using the readers and writers set in the test fixtures in the progress logger. This PR fixes that. It also modifies `TestAccAbortBind`, which was implicitly relying on the bug. I encountered this bug while working on #1672. ## Tests Manually. From non-tty: ``` Error: failed to bind the resource, err: This bind operation requires user confirmation, but the current console does not support prompting. Please specify --auto-approve if you would like to skip prompts and proceed. ``` From tty, bind works as expected. ``` Confirm import changes? Changes will be remotely applied only after running 'bundle deploy'. [y/n]: y Updating deployment state... Successfully bound databricks_pipeline with an id '9d2dedbb-f522-4503-96ba-4bc4d5bfa77d'. Run 'bundle deploy' to deploy changes to your workspace ```

CLI: * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)). Bundles: As of this release CLI will show a prompt is if there are configuration changes which will lead to a DLT recreation. Users can skip the prompt by specifying the `--auto-approve` flag * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)). * Add prompt when a pipeline recreation happens ([#1672](#1672)). * Use materialized views in the default-sql template ([#1709](#1709)). * Update templates to latest LTS DBR ([#1715](#1715)). * Make lock optional in the JSON schema ([#1738](#1738)). * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)). * Include a permissions section in all templates ([#1713](#1713)). * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)). * Fixed variable override in target with full variable syntax ([#1749](#1749)). Internal: * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)). * PythonMutator: explain missing package error ([#1736](#1736)). * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)). * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)). Dependency updates: * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)). --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

Fix streaming of stdout, stdin, stderr in cobra test runner
d5dfcf0

shreyas-goenka marked this pull request as ready for review September 2, 2024 12:24

shreyas-goenka requested review from andrewnester, lennartkats-db and pietern and removed request for lennartkats-db September 2, 2024 12:25

pietern reviewed Sep 2, 2024
View reviewed changes

Comment threadinternal/bundle/helpers.go Outdated
Comment threadinternal/bundle/bind_resource_test.go Outdated

shreyas-goenka commented Sep 2, 2024
View reviewed changes

do not black box
2135a3f

shreyas-goenka requested a review from pietern September 2, 2024 12:55

pietern approved these changes Sep 2, 2024
View reviewed changes

shreyas-goenka added this pull request to the merge queue Sep 2, 2024

Merged via the queue into main with commit 0961236Sep 2, 2024

shreyas-goenka deleted the fix/stream-progress branch September 2, 2024 13:50

andrewnester mentioned this pull request Sep 5, 2024
[Release] Release v0.228.0 #1752
Merged

Navigation Menu

Uh oh!

Fix streaming of stdout, stdin, stderr in cobra test runner - #1742

Fix streaming of stdout, stdin, stderr in cobra test runner#1742
shreyas-goenka merged 2 commits into
mainfrom
fix/stream-progress

shreyas-goenka commented Sep 2, 2024•
edited
Loading

Uh oh!

shreyas-goenka commented Sep 2, 2024

Uh oh!

pietern left a comment

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka left a comment •
edited
Loading

Uh oh!

shreyas-goenkaSep 2, 2024

Uh oh!

shreyas-goenkaSep 2, 2024

Uh oh!

shreyas-goenka commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

shreyas-goenka commented Sep 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

shreyas-goenka commented Sep 2, 2024

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaSep 2, 2024

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaSep 2, 2024

Choose a reason for hiding this comment

Uh oh!

shreyas-goenka commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shreyas-goenka commented Sep 2, 2024•
edited
Loading

shreyas-goenka left a comment •
edited
Loading