Support Workflow Update as a Nexus Operation#783
Conversation
Evanthx
force-pushed
the
nexus-update-workflow
branch
from
f89c889 to
0731aa9
Compare
| { | ||
| DevServerOptions = new() | ||
| { | ||
| DownloadVersion = "v1.7.2-standalone-nexus-operations", |
There was a problem hiding this comment.
Why did you change this? I think it will break SANO tests
There was a problem hiding this comment.
It was still there but moved a few lines lower. But I tested without these changes and things still do work so I don't think these were needed after all and removed them
| /// Gets a value indicating whether this environment supports UpdateWorkflow-backed Nexus | ||
| /// operations, which require the server to deliver update-completion callbacks | ||
| /// (<c>history.enableUpdateCallbacks</c>) together with a Nexus callback-endpoint template. | ||
| /// The pinned local dev server does not yet support these; once it does, add those flags to the |
There was a problem hiding this comment.
The local dev server should support this, if it doesn't we can't merge this anyway so this logic seems unnecessary since the local dev server needs to support it
| public string WorkflowId { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the workflow run ID extracted from the operation token. May be empty. |
There was a problem hiding this comment.
This should never be empty here. We should read the value the server gives back on the update response.
There was a problem hiding this comment.
Comment removed. I was reading the spec and that said the RunID might be empty in a Workflow Update Token - so that is why I had accounted for that use case.
| }); | ||
| } | ||
|
|
||
| private void SkipIfUpdateCallbacksUnsupported() |
There was a problem hiding this comment.
Looking at the other dotnet SDK, we don't skip tests like this and I think it is a dangerous to have logic like this in the tests since we might accidentally skip these tests and loose there coverage. I'll defer to @jmaeagle99 but my preference is to not have this logic
| /// capable server to run them — no code change here required. | ||
| /// </para> | ||
| /// </summary> | ||
| public class NexusUpdateOperationTests : WorkflowEnvironmentTestBase |
There was a problem hiding this comment.
I think we are missing some tests for if the update ID is already completed? Two things I think we should test are sending two Nexus Operations, one after the other has completed, that use the same update ID.
There was a problem hiding this comment.
If we already have these tests then please feel free to ignore
Co-authored-by: Quinn Klassen <klassenq@gmail.com>
Evanthx
force-pushed
the
nexus-update-workflow
branch
from
84cbc08 to
59a284e
Compare
|
The dev server is missing the DC for this feature https://github.com/temporalio/sdk-go/pull/2417/files#diff-265865b1cb4e06ecf08b23b205f709fb4281ba4d4804cbe46f8b91a725073fd5R171 and then we can remove all the test skipping |
3 participants
The linter failed as I added three StartUpdateWorkflowAsync overloads to ITemporalNexusClient and that gets flagged. I suppressed the errors, just calling this out here to make sure that was the right thing to do. My understanding is that when PackageValidationBaselineVersion gets bumped the suppressions will remove themselves.