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

Fix workflow variable scope inconsistency #5558

Merged
merged 16 commits into from
Jun 10, 2024
Merged

Fix workflow variable scope inconsistency #5558

merged 16 commits into from
Jun 10, 2024

Conversation

sfmskywalker
Copy link
Member

@sfmskywalker sfmskywalker commented Jun 8, 2024

This PR ensures that workflow level variables are scoped at the root Workflow Activity Execution Context level, instead of the Workflow Execution Context level. This streamlines the way variables are managed per activity execution context scope and fixes bugs such as reported in #5547

Fixes #5547


This change is Reviewable

The core change in this commit is the refactoring of the handling of variables within the ExpressionExecutionContextExtensions. The methods GetVariable, CreateVariable, and GetVariableBlock have been altered for clarity and simplified reducing redundant code. Unnecessary parameters and returns in method documentation have been removed, and overall code formatting has been improved to enhance readability.
The creation of the MemoryRegister object in the file Workflow.cs was simplified to one line. The previous method, which declared a new object then called the Declare method before returning, was removed.
Added a line of code in the Elsa.Server.Web program.cs file to map controller routes. This change ensures that HTTP requests are correctly directed to their corresponding controller actions.
The logic in the WorkflowStateExtractor has been updated to retain the root Workflow activity context even if it's completed. This change is necessary to keep workflow-level variables accessible.
The RequiresUnreferencedCode attribute was removed from the ConvertTo method in the ObjectConverter class.
Unused services in the AutoUpdateTests.cs class were removed, reducing clutter and improving code readability. Additionally, the DeleteWorkflow_Clustered.cs test file has been renamed to DeleteWorkflowClustered.cs for better naming consistency.
This commit introduces new component tests for simulations involving counters. It includes a new CountdownStep activity that decrements a counter variable, as well as a CountdownWorkflow which consists of a loop based on the aforementioned activity. It also involves a CountdownWorkflowTests class for testing counter persistence across workflow runs.
This commit eliminates the superfluous whitespace in the CountdownWorkflowTests.cs file. It maintains the proper formatting and ensures code consistency across the test component.
Simplified the constructor of the CountdownWorkflowTests class. The changes remove the unnecessary constructor body and pass the 'app' object directly to the base AppComponentTest class, enhancing the code's readability and maintainability.
A new enum ApplicationRole has been added for distinguishing among different roles (Hybrid, Api, Worker) an application can take. In the configuration of MassTransit, it is now possible to disable the consumers based on application role, which can help optimize the usage of resources and increase application efficiency.
Switch to Memory broker
Simplify DisableConsumers assignment.
Rename Hybrid to Default.
Copy link
Contributor

@raymonddenhaan raymonddenhaan left a comment

Choose a reason for hiding this comment

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

You seem to have included the branch with the add application roles changes as well

@sfmskywalker
Copy link
Member Author

Indeed, not on purpose.

@sfmskywalker sfmskywalker merged commit 0422435 into main Jun 10, 2024
3 checks passed
@sfmskywalker sfmskywalker deleted the bug/5547 branch June 10, 2024 08:51
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.

[BUG] Restore variables values after resume from a bookmark.
2 participants