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

Updating hello-kubernetes quickstart to support Endpoint env vars #1030

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

salaboy
Copy link
Contributor

@salaboy salaboy commented Jun 6, 2024

Description

I've updated the examples to use the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variables that are used by the SDKs. If DAPR_HTTP_PORT needs to be also supported I will need some help to validate the code.

I am not sure how images are created for these applications, help is appreciated to validate these changes.

Issue reference

We strive to have all PRs being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1029

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • The quickstart code compiles correctly
  • You've tested new builds of the quickstart if you changed quickstart code
  • You've updated the quickstart's README if necessary
  • If you have changed the steps for a quickstart be sure that you have updated the automated validation accordingly. All of our quickstarts have annotations that allow them to be executed automatically as code. For more information see mechanical-markdown. For user guide with examples see Examples.

@salaboy
Copy link
Contributor Author

salaboy commented Jun 6, 2024

Team, I am not entirely sure how to run the javascript validation locally.. any hints?

@salaboy
Copy link
Contributor Author

salaboy commented Jun 18, 2024

@paulyuk can you help me out here? I wonder if you can advise me on these changes. I can see the Javascript one failing, but it is not clear to me why.

@paulyuk
Copy link
Contributor

paulyuk commented Jun 18, 2024

@salaboy definitely do not worry about Javascript* which has nothing to do with this change. I am separately investigating that. I just kicked off some fresh workflow/action runs in master to see what is going on. Tutorials is the most fragile part of quickstarts - complex and sometimes legacy.

@paulyuk
Copy link
Contributor

paulyuk commented Jun 18, 2024

I see what looks like a timing issue in Hello-Kubernetes where the nodeapp is not started for about 34 loops (seconds) and then starts responding. It looks like some timing issue before the deployment is ready. I have not figured out why yet, just sharing what I'm seeing.

Here is exactly where it fails
https://github.com/dapr/quickstarts/actions/runs/9571684813/job/26389335806

@paulyuk
Copy link
Contributor

paulyuk commented Jun 18, 2024

Now I'm running on my local machine simply using Kind + dapr init -k --dev, and then dapr run -f . -k per readme. It fails in the same way, with ~31 tries failing to find and invoke nodeapp and then it starts working after that time. Those failures will fail the test. We need to focus on the timing and why the first 31 invokes fail.

@paulyuk
Copy link
Contributor

paulyuk commented Jun 18, 2024

I just went all the way back to Dapr 1.11 repo and runtime. The issue repro happens all the way back til then. I believe this is a simple timing issue exposed by dapr.yaml (multi app run). because both services (nodeapp server and pythonapp client) start super fast, and expect to communicate. But in reality nodeapp is not ready for ~31 seconds, and without a readiness check it will fail fast over and over, and the test will break.

I'd love to have a readiness check that can be leveraged by dapr.yaml and the app. If not we'll have to think of workarounds, which would refactor dapr.yaml or revert back to individual dapr run commands that build in a human wait for readiness.

@msfussell
Copy link
Member

@salaboy - The code changes look good. Are you also going to update the text in the readme to have a section on using Dapr Shared with these applications?

@salaboy
Copy link
Contributor Author

salaboy commented Jun 25, 2024 via email

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.

LGTM

@msfussell msfussell merged commit 2b78b3a into dapr:master Jun 27, 2024
7 of 9 checks passed
paulyuk pushed a commit that referenced this pull request Jul 29, 2024
@marcduiker
Copy link
Contributor

@holopin-bot @salaboy Thanks!

Copy link

holopin-bot bot commented Aug 15, 2024

Congratulations @salaboy, the maintainer of this repository has issued you a badge! Here it is: https://holopin.io/claim/clzvblcdj30100cl4xs6dend5

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

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.

hello kubernetes examples (nodeapp and pythonapp) are not using the SDKs
4 participants