Skip to content

Conversation

l0lcat
Copy link
Contributor

@l0lcat l0lcat commented Feb 9, 2025

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Added annotation WorkflowInstanceIdInputParam to the SDK. This can be used on a String parameter of a WorkerTask method. The parameter will be assigned the value of the workflowInstanceId if it is annotated with the WorkflowInstanceIdInputParam.

The workflowInstanceId is useful for persisting with entities in a data store and for logging purposes. The alternative is to have a Task type parameter and get it from there, but it cannot be combined with the InputParam annotation. It's also more elegant to use an annotation.

Alternatives considered

Describe alternative implementation you have considered

@v1r3n v1r3n requested a review from jmigueprieto February 11, 2025 07:23
@l0lcat
Copy link
Contributor Author

l0lcat commented Feb 12, 2025

@jmigueprieto In the presented implementation the workflow instance id is bound to a String parameter. Perhaps it's better and nicer to bind it to a UUID parameter.

Even both could be supported: if the parameter is type String, just bind it. If the parameter type is UUID, parse it and bind it.

Let me know what you think!

@l0lcat String is correct because the id scheme is UUID today but could be different as well. ID generator is pluggable component which means someone change it to generate ids which are unique but does not conform to UUID spec.

@l0lcat l0lcat force-pushed the add-input-parameter-binding-for-workflow-instance-id branch from ed6a90a to b0645be Compare February 14, 2025 20:26
@l0lcat
Copy link
Contributor Author

l0lcat commented Mar 1, 2025

@v1r3n, I removed the support of UUID parameter for assigning the workflowId based on feedback (we can't be sure it wil always be a UUID).

@v1r3n v1r3n merged commit c50c60c into conductor-oss:main Mar 8, 2025
1 check passed
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.

2 participants