[Nexus] Set OnConflictOptions for WorkflowRunOperation

[rodrigozhou]

What was changed

Add OnConflictOptions to StartWorkflowOptions, and set it for Nexus WorkflowRunOperation.

Tests are done in #1828

Why?

Enabling attaching callbacks to running Nexus workflows.

Checklist

1. Closes

2. How was this tested:

3. Any docs updates needed?

[cretz]

@rodrigozhou - I see this is Go SDK only? Do you have plans to author this for other SDKs (at least Java if Nexus only)? We don't consider Go any more important than others. If we want this feature in all SDKs, without favoritism, can you open an issue at https://github.com/temporalio/features and we can add this to all SDKs (including Go) when prioritized?

[bergundy]

LGTM but we'll need a tagged api before merging this.

[cretz]

but we'll need a tagged api before merging this.

I'd argue we need a test confirming this even works before merging. If it's not going to be in a stable dev server release anytime soon, we can discuss a dev server RC release.

[bergundy]

but we'll need a tagged api before merging this.

I'd argue we need a test confirming this even works before merging. If it's not going to be in a stable dev server release anytime soon, we can discuss a dev server RC release.

@rodrigozhou can you add a test in the server for now with this SDK version? There's going to be a server 1.27.0 RC fairly soon, we can open an issue to add a test SDK side once that's ready.

@cretz also note that there are existing server tests for this functionality, they just don't cover the e2e flow with SDK.

[Quinn-With-Two-Ns]

Maybe I just missed it, but shouldn't some user facing SDK docs be updated as part of this PR? This is a new feature, but nothing in docs describe this new behaviour.

[Quinn-With-Two-Ns]

Don't we need workflow test environment support?

[bergundy]

Maybe I just missed it, but shouldn't some user facing SDK docs be updated as part of this PR? This is a new feature, but nothing in docs describe this new behaviour.

Yeah, good callout, this should be documented on the temporalnexus methods (e.g. NewWorkflowRunOperation+WithOptions, ExecuteWorkflow, ExecuteUntypedWorkflow).

[bergundy]

Don't we need workflow test environment support?

I'd say yes but low priority. We can track this separately.

[Quinn-With-Two-Ns]

I consider workflow test environment support a requirement

edit: for GA

[cretz]

they just don't cover the e2e flow with SDK.

I think we need an e2e flow with the SDK (should be no rush to ship something to our users that doesn't work in a server they can use anyways). Obviously it's more of a smoke test than full coverage, but it reasonable to confirm it works from where the user uses it.

[rodrigozhou]

@cretz @Quinn-With-Two-Ns I created a separate PR (#1828) that adds supports for this in the test framework, and added some tests there as well.

[cretz]

I created a separate PR (#1828) that adds supports for this in the test framework, and added some tests there as well.

I would like to make sure the integration test in that PR passes before merging this into master (feel free to move that integration test from there to here since it is unrelated to the test suite stuff done in that PR)

[cretz]

Still need @Quinn-With-Two-Ns approval

[Quinn-With-Two-Ns]

Now that we have the CLI release can we add integration tests?

[rodrigozhou]

@Quinn-With-Two-Ns I added in #1828.
However, it needs the latest server tag, which is not in the latest CLI tag.

[Quinn-With-Two-Ns]

@rodrigozhou you mean v1.3.0-versioning.0 does not work?

[rodrigozhou]

Yes, v1.3.0-versioning.0 uses v1.27.0-128.4, but it needs a fix that in the main branch, but not in the cloud branch.

[cretz]

Mentioned in comment above (though I marked approved since thinking it was there which I now see it's not), I think for things to get to master we need a working integration test to get merged with it

[rodrigozhou]

@cretz I'm gonna create a new tag for the CLI, and update the reference here in the SDK for the tests in the other PR to pass. I think this will resolve.