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

Added sagaexecutor example #489

Closed
wants to merge 26 commits into from
Closed

Added sagaexecutor example #489

wants to merge 26 commits into from

Conversation

stevef1uk
Copy link

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

@stevef1uk stevef1uk requested a review from a team as a code owner December 27, 2023 13:12
@stevef1uk
Copy link
Author

stevef1uk commented Dec 27, 2023

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
Copy link
Author

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
Copy link
Member

mikeee commented Dec 30, 2023

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.

Copy link

codecov bot commented Dec 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevef1uk
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@stevef1uk
Copy link
Author

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
Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Please do not include binary files.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Removed errant binary

@@ -0,0 +1,30 @@
# -*- mode: Python -*-
Copy link
Member

Choose a reason for hiding this comment

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

Please convert it to Makefile, which is more common.

Copy link
Author

@stevef1uk stevef1uk Jan 8, 2024

Choose a reason for hiding this comment

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

Not keen on doing that. However, ChatGPT makes that easily doable :-)

Copy link
Author

@stevef1uk stevef1uk Jan 8, 2024

Choose a reason for hiding this comment

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

Makefiles written (mostly) by ChatGPT, tweaked. Testing found issues but fixed & tested on Mac (M1 CPU) and tested build ok with AMD CPU

Copy link
Author

Choose a reason for hiding this comment

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

Tested fixed makefiles on M2 Mac deployed to RPI k3s cluster & Intel Mac with Rancher desktop. All working as expected so I have finished the Makefiles addition request,

stevef1uk and others added 16 commits January 8, 2024 12:13
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@users.noreply.github.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Just tested on an intel system and realised the README was slightly wrong.

Signed-off-by: stevef1uk <stevef1uk@users.noreply.github.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
…plus more simplifications

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
@stevef1uk
Copy link
Author

Anyone got the time to finish review & merge?

@yaron2
Copy link
Member

yaron2 commented Jan 19, 2024

Anyone got the time to finish review & merge?

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

@mikeee
Copy link
Member

mikeee commented Jan 20, 2024

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

@stevef1uk
Copy link
Author

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.

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

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

@stevef1uk
Copy link
Author

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
Copy link
Member

mikeee commented Feb 2, 2024

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
Copy link
Author

stevef1uk commented Feb 2, 2024

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 :-)

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

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.

I've not commented on the use of snake-case as a convention as this is a sample and is perfectly acceptable and valid.

The comments are a mix of spelling/corrections. A few things I've noticed are:

  • 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.
  • 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.
  • Some absolute paths are specified which should be updated to relative ones.

Looks great, with the minor tweaks it would be a great example to get people started with the saga pattern.

@@ -22,3 +22,5 @@ vendor
# docs
golang.org
coverage.txt

.dapr
Copy link
Member

Choose a reason for hiding this comment

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

What is this new excludes for - can this be a change limited to that of the example folder for which this PR is made?

Copy link
Author

Choose a reason for hiding this comment

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

What is this new excludes for - can this be a change limited to that of the example folder for which this PR is made?

This change was to stop the logs created when running locally from being committed to GitHub. I can delete the .gitignore from this directory though.

Copy link
Member

Choose a reason for hiding this comment

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

I think this licence file should be removed

2. The Saga Subscriber
3. The Saga Poller

The Saga components are shown in Green and the Dapr building blockes in Blue.
Copy link
Member

Choose a reason for hiding this comment

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

sp.

Suggested change
The Saga components are shown in Green and the Dapr building blockes in Blue.
The Saga components are shown in Green and the Dapr building blocks in Blue.

### Prerequisites
1. A kubernetes cluster is required with dapr installed (dapr init -k)
2. Redis & Postgres must be installed on the cluster
3. Tilt is is used to deply the components (see: https://tilt.dev). However, Makefiles are provided as well. Please note that these files
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
3. Tilt is is used to deply the components (see: https://tilt.dev). However, Makefiles are provided as well. Please note that these files
3. Tilt is is used to deploy the components (see: https://tilt.dev). However, Makefiles are provided as well. Please note that these files

sp.

== APP - sagapoller == 2024/01/01 09:14:26 Returned 0 records
```

You will need to manally delete the postgres container for a full clean-up.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You will need to manally delete the postgres container for a full clean-up.
You will need to manually delete the postgres container for a full clean-up.

sp.

@@ -0,0 +1,61 @@
module github.com/stevef1uk/sagaexecutor
Copy link
Member

Choose a reason for hiding this comment

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

This module definition needs to be updated along with any references from each package.

Copy link
Author

Choose a reason for hiding this comment

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

This module definition needs to be updated along with any references from each package.

Replaced all github.com/stevef1uk/sagaexecutor to github.com/dapr/go-sdk/examples/sagaexecutor

"fmt"
"net/http"

//"fmt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//"fmt"

Unused comment

Copy link
Member

Choose a reason for hiding this comment

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

Appears to be a duplicate spec that could be removed.

Copy link
Author

Choose a reason for hiding this comment

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

I have deleted it, but it was there for my convenience as to run on k8s it needs to be edited,

version: v1
metadata:
- name: secretsFile
value: /Users/stevef/dev/sagaexecutor/components/secrets.json
Copy link
Member

Choose a reason for hiding this comment

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

This absolute path to the secrets file appears to be problematic for those attempting to use this. A change required to this file is not documented so I believe it should be relative.

Copy link
Author

Choose a reason for hiding this comment

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

This absolute path to the secrets file appears to be problematic for those attempting to use this. A change required to this file is not documented so I believe it should be relative.

I will look at this later when I have recreated a K8s system to test it against.

Copy link
Author

Choose a reason for hiding this comment

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

This absolute path to the secrets file appears to be problematic for those attempting to use this. A change required to this file is not documented so I believe it should be relative.

I will look at this later when I have recreated a K8s system to test it against.

This absolute path was required for local testing and relative paths wouldn't let me load the component. I have added some code to the setup local.sh file to update this hard path to the one required so that should now work for you.

Comment on lines 40 to 47
/*pp_id string `json:"app_id"`
Service string `json:"service"`
Token string `json:"token"`
callback_service string `json:"callback_service"`
Params string `json:"params"`
Timeout int `json:"timeout"`
Event bool `json:"event"`
LogTime time.Time `json:"logtime"`*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment required?

@stevef1uk
Copy link
Author

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 :-)

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
@stevef1uk
Copy link
Author

stevef1uk commented Feb 5, 2024

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.

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
@stevef1uk
Copy link
Author

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

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
Signed-off-by: stevef1uk <stevef1uk@gmail.com>
@stevef1uk
Copy link
Author

stevef1uk commented Feb 5, 2024

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.

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
…ects stats

Signed-off-by: stevef1uk <stevef1uk@gmail.com>
@stevef1uk
Copy link
Author

Je vous attends encore.

Signed-off-by: mikeee <hey@mike.ee>
@stevef1uk stevef1uk closed this May 5, 2024
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.

4 participants