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

Adding tutorial based on hello-kubernetes #52

Merged
merged 14 commits into from
Jul 23, 2024
Merged

Adding tutorial based on hello-kubernetes #52

merged 14 commits into from
Jul 23, 2024

Conversation

salaboy
Copy link
Collaborator

@salaboy salaboy commented Jun 7, 2024

This PR adds a tutorial that uses the same example as the official quickstart title hello-kubernetes but introducing Dapr Shared. To merge this PR first we need to make sure that those examples support setting a remote Dapr Endpoint -> dapr/quickstarts#1030

Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
@salaboy
Copy link
Collaborator Author

salaboy commented Jun 21, 2024

@yaron2 @alicejgibbons I need someone to approve this PR :)

@salaboy salaboy mentioned this pull request Jun 26, 2024
7 tasks
Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

@salaboy - Reviewed this PR. PTAL

docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
docs/tutorial/README.md Outdated Show resolved Hide resolved
```
{ "orderId": "42" }
```

Copy link
Member

Choose a reason for hiding this comment

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

I would really encourage adding a K8s command here illustrate where the dapr instances are running. Given these are Dapr deployments and this is a default 3 node Kind cluster, you can show that these instance may be running on different nodes compared to where the app is running. In other words, how do you teach someone that Dapr Shared is installed correctly, or how do I look at a cluster and see that this has dapr shared running on it compared to having side car deployment?

Choose a reason for hiding this comment

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

Agree here! Daprd is being deployed as a Deployment right? I was unsure if deployment or DS initially.

Maybe just add a screenshot of the Daprd's running on the cluster at least?

Copy link
Member

Choose a reason for hiding this comment

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

Would it still be useful to add this (pizza demo) as another example in the repo under and example section or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we close this one first and then we can align the Pizza tutorial to follow the same approach?

Choose a reason for hiding this comment

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

Agree on this and told Salaboy this but the pizza one was failing right now.

salaboy and others added 12 commits June 27, 2024 07:38
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Copy link

@alicejgibbons alicejgibbons left a comment

Choose a reason for hiding this comment

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

Ran through this all and worked for me!

Let's create a new Dapr Shared instance for the Node (`nodeapp`) application:

```sh
helm install nodeapp-shared oci://docker.io/daprio/dapr-shared-chart --set shared.appId=nodeapp

Choose a reason for hiding this comment

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

I feel like the helm chart here abstracts what is actually happening too much. Can you add a link to the helm chart at least so that users can inspect the values?

```
{ "orderId": "42" }
```

Choose a reason for hiding this comment

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

Agree here! Daprd is being deployed as a Deployment right? I was unsure if deployment or DS initially.

Maybe just add a screenshot of the Daprd's running on the cluster at least?

Choose a reason for hiding this comment

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

Agree on this and told Salaboy this but the pizza one was failing right now.

@alicejgibbons
Copy link

This good to be merged? @salaboy

@salaboy
Copy link
Collaborator Author

salaboy commented Jul 19, 2024

I was waiting for an approval

@salaboy salaboy merged commit 4c12ad8 into dapr:main Jul 23, 2024
5 checks passed
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.

3 participants