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

Feat/master/autogeneration #177

Merged
merged 35 commits into from
Jan 23, 2024
Merged

Feat/master/autogeneration #177

merged 35 commits into from
Jan 23, 2024

Conversation

ognjenkatic
Copy link
Collaborator

This PR is a major refactor of the client project. It switches over from hand written client code to semi auto generated (NSwag) to achieve full support for conductor API and make it easier to follow changes. There is a layer of abstraction in between the caller and the auto generated code that makes the method names a bit more meaningful.

We also took the opportunity to move to .Net6 and do some code changes to take advantage of some of the newer language features.

Copy link
Collaborator

@boma96 boma96 left a comment

Choose a reason for hiding this comment

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

Some comments to consider

src/ConductorSharp.Client/ConductorSharp.Client.csproj Outdated Show resolved Hide resolved
src/ConductorSharp.Engine/ExecutionManager.cs Outdated Show resolved Hide resolved
@boma96
Copy link
Collaborator

boma96 commented Jan 11, 2024

@ognjenkatic Also can you put nswag file used to generate models and client into repository

@ognjenkatic ognjenkatic requested a review from boma96 January 12, 2024 21:14
@ognjenkatic
Copy link
Collaborator Author

@ognjenkatic Also can you put nswag file used to generate models and client into repository

This would be two files, as we split it into models and client file. Lets see if we like what we made and do some testing. If it works out, I will add them to the .sourcegen directory.

Copy link
Collaborator

@boma96 boma96 left a comment

Choose a reason for hiding this comment

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

We should test this parallel scenario before making stable release. In following days I plan to introduce some changes so after that we can do proper release

Copy link
Collaborator

@boma96 boma96 left a comment

Choose a reason for hiding this comment

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

@ognjenkatic I just noticed that you used WorkflowTaskType instead of Type in Builders.
According to docs type should be used. Also swagger file contains both type and workflowTaskType for some reason.

@ognjenkatic
Copy link
Collaborator Author

@ognjenkatic I just noticed that you used WorkflowTaskType instead of Type in Builders. According to docs type should be used. Also swagger file contains both type and workflowTaskType for some reason.

Conductor seems to allow setting workflow task type in two ways, either as a string type or enum workflowTaskType.

In the generated models setWorkflowTaskType is a non nullable enum, so if not set to actual task type explicitly on our end it will have the first enum value SIMPLE.
image

@ognjenkatic
Copy link
Collaborator Author

Both string and enum values are now sent for type.

@ognjenkatic ognjenkatic merged commit d28681e into master Jan 23, 2024
1 check passed
@ognjenkatic ognjenkatic deleted the feat/master/autogeneration branch January 23, 2024 12:03
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.

Add support for the correlated fetch endpoints Add more workflow manipulation endpoint support
2 participants