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

Recover from panic gracefully #2353

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Recover from panic gracefully #2353

wants to merge 4 commits into from

Conversation

shreyas-goenka
Copy link
Contributor

Changes

This PR adds a recovery function for panics. This indicates to all users running into a panic that it's a bug and they should report it to Databricks.

Tests

Manually. I added panic(fmt.Errorf("some error message")) to the ConfigureBundleWithVariables function which is the fourth entry in the stack trace.

Error message:

.venv➜  cli git:(panic-r) ✗ ./cli bundle deploy
Error: The Databricks CLI unexpectedly panicked. This should not happen.
Please report this issue to Databricks in the form of a GitHub issue at:
https://github.com/databricks/cli

CLI Version: 0.0.0-dev+2d0963661136

Panic Payload: some error message

Stack Trace:
goroutine 1 [running]:
runtime/debug.Stack()
        /Users/shreyas.goenka/go/pkg/mod/golang.org/[email protected]/src/runtime/debug/stack.go:26 +0x64
github.com/databricks/cli/cmd/root.Execute.func1()
        /Users/shreyas.goenka/cli2/cli/cmd/root/root.go:110 +0xa4
panic({0x103408c40?, 0x14000116260?})
        /Users/shreyas.goenka/go/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:785 +0x124
github.com/databricks/cli/cmd/bundle/utils.ConfigureBundleWithVariables(...)
        /Users/shreyas.goenka/cli2/cli/cmd/bundle/utils/utils.go:21
github.com/databricks/cli/cmd/bundle.newDeployCommand.func1(0x14000164c08?, {0x1040ff1c0?, 0x4?, 0x1030601e7?})
        /Users/shreyas.goenka/cli2/cli/cmd/bundle/deploy.go:44 +0x4c
github.com/spf13/cobra.(*Command).execute(0x14000164c08, {0x1040ff1c0, 0x0, 0x0})
        /Users/shreyas.goenka/cli2/cli/vendor/github.com/spf13/cobra/command.go:985 +0x834
github.com/spf13/cobra.(*Command).ExecuteC(0x14000414608)
        /Users/shreyas.goenka/cli2/cli/vendor/github.com/spf13/cobra/command.go:1117 +0x344
github.com/spf13/cobra.(*Command).ExecuteContextC(...)
        /Users/shreyas.goenka/cli2/cli/vendor/github.com/spf13/cobra/command.go:1050
github.com/databricks/cli/cmd/root.Execute({0x1036e3e00?, 0x1040ff1c0?}, 0x103fd1d68?)
        /Users/shreyas.goenka/cli2/cli/cmd/root/root.go:125 +0x88
main.main()
        /Users/shreyas.goenka/cli2/cli/main.go:13 +0x44

Copy link
Contributor

@denik denik left a comment

Choose a reason for hiding this comment

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

Could we add an acceptance test by either forcing panic somehow of add special flag to inject it?

@shreyas-goenka
Copy link
Contributor Author

Could we add an acceptance test by either forcing panic somehow or adding a special flag to inject it?

We could, but I think it's fine to skip this one. There is no great way to do this since we would have to add a new dummy command or flag just for this.

The goal is to have a QOL improvement for customers and make it clear to them that this is a bug on our side. Even if it fails we are not losing out on anything crucial.

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Could you include an acceptance test for the output?

I'd like to see the diff between the current panic output and the new panic output.

cmd/root/root.go Outdated Show resolved Hide resolved
@denik denik self-requested a review February 13, 2025 11:51
@@ -0,0 +1,13 @@

>>> [CLI] panic
The Databricks CLI unexpectedly panicked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Panic is a golang-specific term that users are not required to know, perhaps "The Databricks CLI had fatal error"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way works. The next line does direct the user to open an issue on Databricks so they don't need to know what panicking means.

Copy link
Contributor

@denik denik left a comment

Choose a reason for hiding this comment

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

(see inline comments)

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