Update domain deprecation command to fail if workflow exists #4126
Conversation
Pull Request Test Coverage Report for Build 3a7dc900-12d6-4fa1-a24c-f4527269a5ec
💛 - Coveralls |
tools/cli/domainCommands.go
Outdated
| // check if there is any workflow in this domain, if exists, do not deprecate | ||
| wfs, _ := listClosedWorkflow(getWorkflowClient(c), 1, 0, time.Now().UnixNano(), "", "", workflowStatusNotSet, nil, c) | ||
| if len(wfs) > 0 { | ||
| ErrorAndExit("Operation DeprecateDomain failed.", errors.New("Workflow history not cleared in this domain.")) |
There was a problem hiding this comment.
why closed workflows still matter? Is that a requirements or some technical workaround?
There was a problem hiding this comment.
it's a requirement. We want to be conservative.
There was a problem hiding this comment.
May I ask what's purpose of this change? I thought only start or signal workflow is blocked for deprecated domains. Trying to understand why do closed workflow and running workflow matter here?
If we want to enforce this requirement that domain deprecation only happens for domains in which all workflows have passed retention, I think this check should be done inside the handler of domain deprecation call instead of just in the CLI?
There was a problem hiding this comment.
The reason I made the change in CLI but not in server is to let the caller do the check.
And within CLI, we can add another flag (e.g. --force) to make this check optional, but the default behavior is to be conservative and does the check.
There was a problem hiding this comment.
@meiliang86 What's your opinion on this? I prefer to adding the check in CLI and later we can add a flag to skip the check if we'd like to.
There was a problem hiding this comment.
Yeah adding the check to the CLI is fine. Technically we can deprecate domains even there's open/close workflows, so it's ok to not check it on the server side.
This extra check is just to make sure we don't shoot ourselves in the foot (i.e. accidentally deprecating domains that are still in use), as right now we don't support resurrecting domains once deprecated.
Shaddoll
force-pushed
the
cli-dep
branch
2 times, most recently
from
0b1448c
to
98db25d
Compare
|
|
||
| ctx, cancel := newContext(c) | ||
| defer cancel() | ||
|
|
||
| if !force { | ||
| // check if there is any workflow in this domain, if exists, do not deprecate | ||
| wfs, _ := listClosedWorkflow(getWorkflowClient(c), 1, 0, time.Now().UnixNano(), "", "", workflowStatusNotSet, nil, c) |
There was a problem hiding this comment.
do we need to check the error and block deprecation when failed to verify workflows?
There was a problem hiding this comment.
Yes, it's done within the helper functions.
There was a problem hiding this comment.
Oh I see. I thought the second return value is error. My bad.
|
|
||
| ctx, cancel := newContext(c) | ||
| defer cancel() | ||
|
|
||
| if !force { | ||
| // check if there is any workflow in this domain, if exists, do not deprecate | ||
| wfs, _ := listClosedWorkflow(getWorkflowClient(c), 1, 0, time.Now().UnixNano(), "", "", workflowStatusNotSet, nil, c) |
There was a problem hiding this comment.
Oh I see. I thought the second return value is error. My bad.
What changed?
Update the domain deprecation command to check if there exists any workflow history in that domain before deprecating the domain by default. And also add a new flag '--force' to allow user to skip the workflow history check.
Why?
We want to be cautious when deprecating a domain, so the default behavior is not to do anything if workflow history exists. We also want to keep the original behavior to deprecate a domain when we don't care workflow history in that domain.
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes