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 command to create Durable Task helpers #947

Closed
wants to merge 12 commits into from

Conversation

stuartleeks
Copy link
Contributor

@stuartleeks stuartleeks commented Dec 24, 2018

Initial implementation for #946 for review/comment.

Currently handles generating helpers for activities and sub-orchestrations - considering whether to add StartNew helpers.

The PR adds a single new command func durable create-helpers. This command doesn't require any arguments as it picks up all the information from the function project.

As with other commands in the CLI, this expects to be run from the function project folder.

@stuartleeks
Copy link
Contributor Author

@cgillum - would love your thoughts on this :-)

Copy link
Contributor

@ahmelsayed ahmelsayed left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments from me. @cgillum, @glennamanns or @kashimiz would know better than me regarding the function aspects of the change.

@@ -0,0 +1,264 @@
## Ignore Visual Studio temporary files, build results, and
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a .gitignore per scenario here? seems a bit too much :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I'll look at moving that up a level to avoid one per scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done in the latest commits

if (workerRuntime != WorkerRuntime.dotnet)
{
ColoredConsole.WriteLine(ErrorColor($"create-durable-helpers does not support runtime: {workerRuntime}"));
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

throw here so that the exit code is set correctly, or set the exit code yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to set the exit code

@stuartleeks
Copy link
Contributor Author

@ahmelsayed - I believe the changes you requested are made in the latest commits. Let me know if you have any further comments

@cgillum, @glennamanns, @kashimiz - would love your thoughts :-)

@stuartleeks stuartleeks force-pushed the durable-task-helpers branch 2 times, most recently from 9ed4cc3 to 8282b14 Compare January 21, 2019 19:19
@stuartleeks
Copy link
Contributor Author

I have now added the SubOrchestrator helper generation.
Also rebased against the latest master and re-run tests.

<PackageReference Include="Buildalyzer.Workspaces" Version="2.0.0" />
<PackageReference Include="Microsoft.CodeAnalysis" Version="2.9.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="2.9.0" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic.Workspaces" Version="2.9.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmelsayed Just to confirm, you're good with these new CLI dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Buildalyzer as Roslyn doesn't currently have support for MSBuildWorkspace (the type that let's you create a workspace from a csproj file) in .NET Core: dotnet/roslyn#17974

Buildalyzer source is here under an MIT license.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the delay, just saw these comments. I think the runtime already brings in the CodeAnalysis packages. I don't really have any particular concerns unless @fabiocav does.

@ahmelsayed ahmelsayed changed the base branch from master to dev January 23, 2019 01:14
@stuartleeks
Copy link
Contributor Author

A quick note from a conversation with @cgillum:

This PR only works for dotnet functions and will give an error indicating that it is not supported for other languages (yet!). Additionally, since this check is made via the functions settings this command will only support Functions v2.

@stuartleeks
Copy link
Contributor Author

Just rebased on dev as I see @ahmelsayed switched the PR branch there :-)

@stuartleeks
Copy link
Contributor Author

Is there any more feedback on this? :-)

@ahmelsayed
Copy link
Contributor

I'm closing this old PR. If this is something that we still need to look at, please work with @fabiocav to get this rebased and merged.

@ahmelsayed ahmelsayed closed this Oct 15, 2019
@lock
Copy link

lock bot commented Dec 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants