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

Add DryRun endpoint and workflow instance finder #5110

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Conversation

sfmskywalker
Copy link
Member

@sfmskywalker sfmskywalker commented Mar 20, 2024

Added a new DryRun endpoint for executing alteration plans and implemented the IWorkflowInstanceFinder interface to search for workflow instances. Refactored existing code to use new workflow instance finder service, resulting in cleaner activity implementation.

Fixes #5109

Added a new DryRun endpoint for executing alteration plans and implemented the IWorkflowInstanceFinder interface to search for workflow instances. Refactored existing code to use new workflow instance finder service, resulting in cleaner activity implementation.
@sfmskywalker sfmskywalker requested review from a team March 20, 2024 21:57
Change the conditional logic to use `IsEmpty` method on filters rather than checking if the collection `workflowInstanceIds` is empty. This ensures that matching workflow instances are correctly identified when no specific filters are set.
The WorkflowInstanceFinder service has been refactored to include a default workflow status of Running and to utilize a new method WorkflowFilterIsEmpty, which checks if the workflow instance filter is empty. This improves code readability and encapsulates the logic for determining an empty filter.
@sfmskywalker sfmskywalker merged commit d117543 into main Mar 21, 2024
6 checks passed
@sfmskywalker sfmskywalker deleted the issue/5109 branch March 21, 2024 07:22
var workflowInstanceFilterIsEmpty = WorkflowFilterIsEmpty(workflowInstanceFilter);

var workflowInstanceIds = workflowInstanceFilterIsEmpty
? Enumerable.Empty<string>().ToHashSet()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we give some feedback to the user that they actually need to enter filter criteria or is this implied from the empty list that is returned?

return workflowInstanceIds;
}

private bool WorkflowFilterIsEmpty(WorkflowInstanceFilter filter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this from the filter itself? There we would be able to re use the logic

}

/// <inheritdoc />
public override async Task HandleAsync(AlterationWorkflowInstanceFilter filter, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be easier to add dryrun as an optional parameter to the alteration endpoint? Now we need to ensure these 2 endpoints stay the same.

@raymonddenhaan
Copy link
Contributor

This closes #5071

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.

Activity Filter in API Not Filtering as Expected
2 participants