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

Refresh pub-sub quickstart (tutorial) fixing routes and stale dependency #539

Merged
merged 22 commits into from
Feb 23, 2022

Conversation

paulyuk
Copy link
Contributor

@paulyuk paulyuk commented Feb 18, 2022

Description

The pub-sub and distributed-calculator quickstart/tutorials are refreshed fixing two classes of issues:

  • There is always a startup error complaining about "<" found when subscribing to /dapr/subscribe/ receives the React client page (RCA is the * route)
  • Migrated HTTP handling to axios library, from legacy request library, to match our new quickstarts and eliminate deprecated dependency.

Issue reference

Fixes #516
Fixed #270

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.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk
Copy link
Contributor Author

paulyuk commented Feb 18, 2022

@greenie-msft or @msfussell please review

@paulyuk paulyuk added the do-not-merge Do not merge the PR label Feb 18, 2022
@paulyuk
Copy link
Contributor Author

paulyuk commented Feb 18, 2022

Looking at test issue

…xpress')

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk
Copy link
Contributor Author

paulyuk commented Feb 19, 2022

We're really close here. I fixed issue so tests pass.

I'm trying to also fix distributed calculator while i'm here to use an http package that is better supported (like axios). Piping requests/responses in Axios is tricky so I'm still working on that.

…ith service invoke.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
…h Axios.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
…@shubham1172 review.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk paulyuk removed the do-not-merge Do not merge the PR label Feb 21, 2022
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
pub-sub/README.md Outdated Show resolved Hide resolved
pub-sub/README.md Outdated Show resolved Hide resolved
shubham1172
shubham1172 previously approved these changes Feb 21, 2022
Copy link
Member

@shubham1172 shubham1172 left a comment

Choose a reason for hiding this comment

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

Thanks @paulyuk, LGTM!

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>
paulyuk and others added 3 commits February 21, 2022 09:30
Co-authored-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk paulyuk changed the title Refresh pub-sub quickstart (tuturial) fixing routes and stale dependency Refresh pub-sub quickstart (tutorial) fixing routes and stale dependency Feb 22, 2022
shubham1172
shubham1172 previously approved these changes Feb 22, 2022
distributed-calculator/README.md Outdated Show resolved Hide resolved
distributed-calculator/README.md Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
distributed-calculator/react-calculator/server.js Outdated Show resolved Hide resolved
pub-sub/README.md Outdated Show resolved Hide resolved
…ng URLs, and removing console.log

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
… to Express.json() built in. Updated dependencies

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@XavierGeerinck
Copy link
Contributor

LGTM

@msfussell
Copy link
Member

@paulyuk - Looks like there is an error on the KubeTests, Any concerns here and have you looked into why?
Step: Deploy Node App
command: kubectl logs --selector app=node-subscriber -c node-subscriber
return_code: 0
duration_seconds: 0.154
Expected stdout (output_match_mode: exact):
A: Message on A
B: Message on B
Actual stdout:
Node App listening on port 3000!

ERROR expected lines not found:
	A:  Message on A
	B:  Message on B

@paulyuk
Copy link
Contributor Author

paulyuk commented Feb 23, 2022

@paulyuk - Looks like there is an error on the KubeTests, Any concerns here and have you looked into why? Step: Deploy Node App command: kubectl logs --selector app=node-subscriber -c node-subscriber return_code: 0 duration_seconds: 0.154 Expected stdout (output_match_mode: exact): A: Message on A B: Message on B Actual stdout: Node App listening on port 3000!

ERROR expected lines not found:
	A:  Message on A
	B:  Message on B

Thanks @msfussell . I think I spot the root cause. In the pub-sub and bindings tests, we try to pull docker images into that k8s cluster via that compose file. This happens over and over again in this automated test.
image

Since it's a public registry and there are no credentials used for the pull, this is an anonymous pull. And it's being throttled! When throttled there is no Kafka in the cluster and then the bindings fail.

Let's chat about migrating images to a GHCR (best practice to be in private registry anyway). We can alternatively sign in to Hub using credentials.

All that said, This PR is passing and ready for merge.

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 0adadc9 into master Feb 23, 2022
msfussell pushed a commit that referenced this pull request Mar 4, 2022
* Bump follow-redirects in /distributed-calculator/react-calculator/client (#530)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.14.7 to 1.14.8.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.14.7...v1.14.8)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump follow-redirects in /pub-sub/react-form/client (#531)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.14.7 to 1.14.8.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.14.7...v1.14.8)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Mark Fussell <mfussell@microsoft.com>

* Refresh pub-sub quickstart (tutorial) fixing routes and stale dependency (#539)

* Fixing error caused by route to * conflicing with /dapr/subscribe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrating from `requests` to `axios` to improve supportability

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changed from " to ' matching existing styles for Javascript

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changing `axios` from import to require('axios'), matching require('express')

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Consistent use of Async enabled http code

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrated distributed calculator from `request` with pipe to `axios` with service invoke.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixed react * route bug that conflicts with /dapr/subscribe route

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Reducing log levels introduced by migration.  added debug flag.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating MM test to match improved precision of Express server.js with Axios.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* updating console.log from console.debug (minor) for consistency.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updated vars to const, and then updated README to match.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating Axios release(s) in Distributed Calc and pub-sub to v0.26 per @shubham1172 review.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Async posts needed await.  Readme's tweaked to match updates.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Async should pair with await

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixing comment

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Removing commented out pipe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review updated JS styles: return, casting, inlining URLs, and removing console.log

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review migrated from BodyParser.json() middleware to Express.json() built in.  Updated dependencies

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Paulyuk/fixpubsuberror (#547)

* Fixing error caused by route to * conflicing with /dapr/subscribe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrating from `requests` to `axios` to improve supportability

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changed from " to ' matching existing styles for Javascript

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changing `axios` from import to require('axios'), matching require('express')

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Consistent use of Async enabled http code

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrated distributed calculator from `request` with pipe to `axios` with service invoke.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixed react * route bug that conflicts with /dapr/subscribe route

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Reducing log levels introduced by migration.  added debug flag.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating MM test to match improved precision of Express server.js with Axios.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* updating console.log from console.debug (minor) for consistency.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updated vars to const, and then updated README to match.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating Axios release(s) in Distributed Calc and pub-sub to v0.26 per @shubham1172 review.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Async posts needed await.  Readme's tweaked to match updates.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Async should pair with await

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixing comment

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Removing commented out pipe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review updated JS styles: return, casting, inlining URLs, and removing console.log

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review migrated from BodyParser.json() middleware to Express.json() built in.  Updated dependencies

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Hotfix to test, given precision sensitivity on test output number

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Improved MM test fix for quickstarts (#548)

* Fixing error caused by route to * conflicing with /dapr/subscribe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrating from `requests` to `axios` to improve supportability

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changed from " to ' matching existing styles for Javascript

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Changing `axios` from import to require('axios'), matching require('express')

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Consistent use of Async enabled http code

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Migrated distributed calculator from `request` with pipe to `axios` with service invoke.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixed react * route bug that conflicts with /dapr/subscribe route

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Reducing log levels introduced by migration.  added debug flag.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating MM test to match improved precision of Express server.js with Axios.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* updating console.log from console.debug (minor) for consistency.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updated vars to const, and then updated README to match.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating Axios release(s) in Distributed Calc and pub-sub to v0.26 per @shubham1172 review.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Async posts needed await.  Readme's tweaked to match updates.

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Update pub-sub/README.md

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Async should pair with await

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixing comment

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Removing commented out pipe

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review updated JS styles: return, casting, inlining URLs, and removing console.log

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Per @XavierGeerinck review migrated from BodyParser.json() middleware to Express.json() built in.  Updated dependencies

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Hotfix to test, given precision sensitivity on test output number

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixing all remaining decimal precision issues with MM tests in readmes, including observability which now fails

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>

* Fix curl sample to work on macOS (#543)

Signed-off-by: Marcin Jahn <marcinj10@gmail.com>

* Fix sample path in readme (#555)

Signed-off-by: Xin Shi <xshi@microsoft.com>

Co-authored-by: Mark Fussell <mfussell@microsoft.com>

* Remove PR check for docker login (#565)

* remove PR check for docker login

Signed-off-by: tanvigour <tanvi.gour@gmail.com>

* remove PR trigger for the workflow to fix docker login issue

Signed-off-by: tanvigour <tanvi.gour@gmail.com>

Co-authored-by: Artur Souza <artursouza.ms@outlook.com>

* Test validation cleanup (#569)

* Test validation cleanup

Signed-off-by: tanvigour <tanvi.gour@gmail.com>

* add one response message in each of the validation steps

Signed-off-by: tanvigour <tanvi.gour@gmail.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Mark Fussell <mfussell@microsoft.com>
Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Co-authored-by: Shubham Sharma <shubhash@microsoft.com>
Co-authored-by: Marcin Jahn <marcinj10@gmail.com>
Co-authored-by: Xin Shi <xshi@microsoft.com>
Co-authored-by: tanvigour <60332928+tanvigour@users.noreply.github.com>
Co-authored-by: Artur Souza <artursouza.ms@outlook.com>
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.

pub-sub react-form quickstart throws error on startup when getting subscriptions Change npm package request
4 participants