Skip to content
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

Remove SetOptions method from builder #178

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

boma96
Copy link
Collaborator

@boma96 boma96 commented Jan 24, 2024

Motivation:
We have two different ways of specifying workflow version.

  • SetOptions method(with additional metadata)
  • VersionAttribute

Main reason VersionAttribute was introduced was to be able to use scaffolded workflows with version greater than 1(i.e. we always assumed workflow had version equal to 1).

Problem with SetOptions:
If the workflow class itself is used directly in another workflow as subworkflow then we do not have ability to read workflow version which was set in SetOptions. In that scenario we had to use VersionAttribute

I have decided to remove SetOptions so scenario above can not happen. Functionality of setting metadata is now achieved using WorkflowMetadataAttribute.

using ConductorSharp.Engine.Builders.Metadata;

namespace ConductorSharp.Engine.Tests.Samples.Workflows;

public class WorkflowMetadataWorkflowInput : WorkflowInput<WorkflowMetadataWorkflowOutput> { }

public class WorkflowMetadataWorkflowOutput : WorkflowOutput { }

[WorkflowMetadata(
    OwnerEmail = "[email protected]",
    Description = "This is description",
    OwnerApp = "Owner",
    FailureWorkflow = typeof(ConditionallySendCustomerNotification)
)]
public class WorkflowMetadataWorkflow(
    WorkflowDefinitionBuilder<WorkflowMetadataWorkflow, WorkflowMetadataWorkflowInput, WorkflowMetadataWorkflowOutput> builder
) : Workflow<WorkflowMetadataWorkflow, WorkflowMetadataWorkflowInput, WorkflowMetadataWorkflowOutput>(builder)
{
    public CustomerGetV1 GetCustomer { get; set; }

    public override void BuildDefinition()
    {
        _builder.AddTask(wf => wf.GetCustomer, wf => new() { CustomerId = 1 });
    }
}

Also, all metadata attribute classes(OriginalName, Version, WorkflowMetadata) are moved into new namespace
ConductorSharp.Engine.Builders.Metadata

@boma96 boma96 requested a review from ognjenkatic January 24, 2024 11:22
Copy link
Collaborator

@ognjenkatic ognjenkatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but i think it might be a bit confusing having three different attributes we use for what is essentially all metadata.

  • Version
  • Owner etc.
  • Original name

Maybe these could all be merged into a single attribute, something like ConductorWorkflow, and to be consistent perhaps have something like ConductorWorker for tasks, maybe these could be used then to find and register these classes automatically for DI. This is only a suggestion however, my thinking is that it would be good to reduce the number of steps in which tasks/workflows are registered/configured to avoid confusion.

@ognjenkatic ognjenkatic merged commit 206595b into master Feb 16, 2024
1 check passed
@ognjenkatic ognjenkatic deleted the feat/master/remove-set-options branch February 16, 2024 08:20
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