search

dapr / go-sdk

Dapr SDK for go
Apache License 2.0
446 stars 171 forks source link

Added sagaexecutor example #489

Closed stevef1uk closed 5 months ago

stevef1uk commented 10 months ago

Please review and see if this project is a suitable example.

stevef1uk commented 10 months ago

Sorry, somehow missed the extra parameter I asked on the NewService call in Poller as that logic hadn't been changed for a while & I must have been testing with an old image without noticing.

stevef1uk commented 10 months ago

Also, I just realised that the Tilt files I have used to build & deploy the components are configured for my RPi k3s cluster as I don't have a X86 one available for me to use.

mikeee commented 10 months ago

Amazing example, I really like this. Will definitely be useful for showcasing tilt given the push for an improvement in the developer experience.

I think it'd be perfect if you could possibly convert the README.pdf to markdown and combine with the existing README.md?

What would be even more brilliant is if we could get a validation test set up for this example too - mechanical markdown example go-sdk/examples/actor/README.md

I know it's an example but I'd like to commit an improved docker build process - with scratch images. Can contribute and review with suggested changes when I have more bandwith.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (04f7b59) 70.08% compared to head (9504d8a) 58.04%. Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #489 +/- ## =========================================== - Coverage 70.08% 58.04% -12.04% =========================================== Files 35 55 +20 Lines 2841 3568 +727 =========================================== + Hits 1991 2071 +80 - Misses 738 1375 +637 - Partials 112 122 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stevef1uk commented 10 months ago

Amazing example, I really like this. Will definitely be useful for showcasing tilt given the push for an improvement in the developer experience.

I think it'd be perfect if you could possibly convert the README.pdf to markdown and combine with the existing README.md?

What would be even more brilliant is if we could get a validation test set up for this example too - mechanical markdown example go-sdk/examples/actor/README.md

I know it's an example but I'd like to commit an improved docker build process - with scratch images. Can contribute and review with suggested changes when I have more bandwith.

Thanks.

I have started on this and just committed to my repo the local set-up. I have written a script to automate the creation of the Postgres container & written a Dapr.yaml file to start the poller, a subscriber and the a test server.

Next I will tackle the markup.

When I boot up my proper dev machines again I will look for how I have previously created docker images from scratch. This will be a few years ago as I gave up on k8s a long time ago.

stevef1uk commented 10 months ago

Ok. I have completed the documentation consolidation.

I can build a scratch image, but I can't figure out how to do that from Tilt.

stevef1uk commented 10 months ago

I have checked my image sizes and they are about 20Mb so I don't think moving to scratch is worth it?

stevef1uk commented 9 months ago

I have just included the latest changes I made, which was to remove my Postgres custom code & use the Dapr Postgres Binding instead. The only example for how to use this was in the Bindings test code ;-) Thus, this may be useful o others?

stevef1uk commented 9 months ago

OK. I have stopped making changes to this base code. Hopefully, it is ok now?

stevef1uk commented 9 months ago

Anyone got the time to finish review & merge?

yaron2 commented 9 months ago

Anyone got the time to finish review & merge?

Yes i'll take a look at this today and early next week.

mikeee commented 9 months ago

A saga example in java was added to the quickstarts repo. I'm wondering if this would better be merged there.

stevef1uk commented 9 months ago

A saga example in java was added to the quickstarts repo. I'm wondering if this would better be merged there.

I can't see that but I did think about whether this should be in the QuickStarts but decided it wasn't a good fit as this is unashamably a Go project.

stevef1uk commented 8 months ago

I think it'd be amazing if you were able to add a git workflow to validate this example continuously

Sorry, that is something I usually leave to the DevOps team once the Cloud environments have been stood up ;-)

mikeee commented 8 months ago

Fair enough, I'm happy to pick that up as long as the example is working and we have an issue raised as a to-do at a later date.

Will review when I get home 👀

stevef1uk commented 8 months ago

Fair enough, I'm happy to pick that up as long as the example is working and we have an issue raised as a to-do at a later date.

Will review when I get home 👀

Cool, although I don't want you to work extra hours. I just realised it has been over 24 years since I stopped being a professional programer. These days I do pretty much what I want and can get hands-on again. I have a new contract starting soon where I will have to learn Python. The last scripting language I used seriously was Perl so that dates me even more :-)

Please let me know if the code doesn't work and I will fix it. I have only been able to test on Macs (Intel & M CPUs) and a couple of types of Kubernetes (Rancher & K3s) types. It worked for me :-)

stevef1uk commented 8 months ago

First look review comments - I've reviewed without running the example:

This example involves a lot of steps which could potentially be added to a main makefile at the root of the example. I don't see an issue with this example being committed in this way but should in the future be refactored with a few issues being raised for this to be completed at a later date.

Ok. I use Tilt to deploy so I kept the Makefile minimal as daixiang0 asked for Makefiles, which I first used in 1987 and assumed were no longer a thing :-)

I've not commented on the use of snake-case as a convention as this is a sample and is perfectly acceptable and valid. Yes a good Go Dev told me I should use golint, which to be honest I didn't this time. As you say it is only an example and I haven't written lots of unit test & used a Mocking framework. As this example is basically heavily using the Dapr Go SDK I focussed on integration tests as it was more fun for me and I am not being paid so I have skipped the boring stuff ;-)

The comments are a mix of spelling/corrections. A few things I've noticed are: Sorry, I find unless I print things out (which no devs do these days) I read what I expect to see and not what is written, so I will fix those.

  • Imports are currently referencing the repo github.com/stevef1uk/sagaexecutor which should be changed to that of this dapr repo (I've only noted this on the go.mod but this affects each package where the import is made. This is something that always confuses me with Go. When I make that change I won't be able to build and test unless I use the replace capability in go.mod, which I think I had to do when I first committed this code to my GitHub repo. Obviously, I can't commit that Go,mod with the replace either.

  • I potentially would like to see an env-var to specify the user's dockerhub/registry to push to which may reduce the amount of changes where seahope needs to be changed. Agreed. I was going to do that but forgot.

  • Some absolute paths are specified which should be updated to relative ones.

Agreed. Looks great, with the minor tweaks it would be a great example to get people started with the saga pattern. Thanks. I may be able to start on this tomorrow. I don't yet have a start date for my return to work and when I do anything I do on this will need to be outside working hours. A bit of coding stuff is always more fun for me than powerpoint decks, but generally not what I am paid for anymore :-)

stevef1uk commented 8 months ago

I have made a start and added a commit for most of the minor changes requested. I haven't tried to recompile after changing the imports yet.

Update: I have compiled the code successfully. You will need to edit the replace in Go.mod for your location to try to build this before its is actually added to the GitHub repo and then that replace can be removed.

stevef1uk commented 8 months ago

Just added a commit to enable DockerHub Id to be configured in the Makefiles. Tested this on my repo ok.

stevef1uk commented 8 months ago

OK, all changes requested in the initial review (that I can see above) and have responded to have been addressed. I hope you can now build & test this locally?

For changing the owning repo name for the docker container images simply set the DOCKER_ID environment variable to it.

stevef1uk commented 8 months ago

Je vous attends encore.