-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Allow for deleting connection in an unexpected state #11302
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
Allow for deleting connection in an unexpected state #11302
Conversation
The unit test should probably all live in the temporal client test file, but for some reason the test kept on hanging when I tried to port the test to that file, so I kept them in the connection manager workflow test file for now. fyi @benmoriceau |
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.
It looks good overall.
I don't think that we need the tests that are in the ConnectionManagerWorkflowTest
class. The fact that we are instantiating the Temporal client in it is a warning to me, we should just check that we call the start with signal or just the signal method base on the existence of the workflow in the temporal client.
Does it sound right to you?
@benmoriceau that works for me. That was my original plan, but i realize that test didn't actually track/check the state of anything. I was going to include both, but I ran into some null pointer exceptions with I think i can figure it out and put it up soon. |
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.
LGTM: Could we think about an AcceptanceTest for this as well?
I just saw the comment about the AcceptanceTest after merging. I can work on one in a different diff. |
What
When a temporal workflow is in an unexpected state, there is no way to delete a connection unless you interact with temporal directly. This PR will make it so that the deletion occurs even if the temporal state is in an unexpected state and addresses issue #11214.
How
If the temporal workflow is in an unexpected state, we spin up a new workflow and send a signal to delete the connection.
Tests
In addition to unit tests, this was tested locally by terminating a temporal workflow with the command
docker run --rm -it --entrypoint tctl --env TEMPORAL_CLI_ADDRESS=host.docker.internal:7233 temporalio/admin-tools:1.14.0 wf terminate --wid <workflow_id>
and then attempting to delete the connection through the UI.