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

feature: Optimized pubsub api demo #594

Merged
merged 10 commits into from
May 26, 2022
Merged

Conversation

codedededi
Copy link
Contributor

What this PR does:
mix pubsub demo

Which issue(s) this PR fixes:

Fixes #584

@mosn-community-bot
Copy link

Hi @gdutChenxj, welcome to mosn community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #594 (4911f00) into main (32d5cd6) will not change coverage.
The diff coverage is n/a.

❗ Current head 4911f00 differs from pull request most recent head a456406. Consider uploading reports for the commit a456406 to get more accurate results

@@           Coverage Diff           @@
##             main     #594   +/-   ##
=======================================
  Coverage   60.92%   60.92%           
=======================================
  Files         120      120           
  Lines        6377     6377           
=======================================
  Hits         3885     3885           
  Misses       2120     2120           
  Partials      372      372           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32d5cd6...a456406. Read the comment docs.

@codedededi codedededi changed the title mix pubsub demo feature: mix pubsub demo May 22, 2022
@codedededi codedededi changed the title feature: mix pubsub demo feature: Optimized pubsub api demo May 22, 2022
@codedededi
Copy link
Contributor Author

  • i have signed CLA but the status is not sync automatically
  • to merge all pubsub demo, some folder will be deleted and it make an error when Quickstart Validation exec cmd cd /home/runner/work/layotto/layotto/demo/pubsub/redis/server/,the script seem to be build on the workflow server,i think it need somebody to update the script or ignore the validation error

@seeflood
Copy link
Member

seeflood commented May 22, 2022

i have signed CLA but the status is not sync automatically

Fixed. I closed and reopened this PR to rerun the workflow .

to merge all pubsub demo, some folder will be deleted and it make an error when Quickstart Validation exec cmd cd /home/runner/work/layotto/layotto/demo/pubsub/redis/server/,the script seem to be build on the workflow server,i think it need somebody to update the script or ignore the validation error

Yes you are right :)
The quickstart doc https://mosn.io/layotto/#/en/start/pubsub/start and https://mosn.io/layotto/#/zh/start/pubsub/start used the redis demo:
image
image
And Layotto runs these quickstart document in the workflow, see https://mosn.io/layotto/#/zh/development/test-quickstart

So, could u help modify the documentation in this PR?
Just modify the doc to use the new demo code.
If you don't know how to fix it after reading the doc, I can help submit a PR to your branch to fix it :)

@codedededi
Copy link
Contributor Author

i will update the doc for the new demo

@codedededi
Copy link
Contributor Author

@seeflood I have updated the quick start doc and it can work well in my local test. But there is a confliction need somebody with write access to handle.

@seeflood
Copy link
Member

seeflood commented May 26, 2022

@seeflood I have updated the quick start doc and it can work well in my local test. But there is a confliction need somebody with write access to handle.

Thanks ! I just saw this message.
You have the permission to resolve the conflicts.
To resolve them, you can:

  1. Merge main branch into your branch, on your computer. The IDE will guide you to fix conflicts
  2. After all resolved and merged, you just push your new code to your forked github repo. And then this PR will be updated automatically

docs/en/start/pubsub/start.md Outdated Show resolved Hide resolved
docs/en/start/pubsub/start.md Outdated Show resolved Hide resolved
docs/zh/start/pubsub/start.md Outdated Show resolved Hide resolved
docs/zh/start/pubsub/start.md Outdated Show resolved Hide resolved
demo/pubsub/README.md Outdated Show resolved Hide resolved
demo/pubsub/README.md Outdated Show resolved Hide resolved
demo/pubsub/README.md Outdated Show resolved Hide resolved
@codedededi
Copy link
Contributor Author

@seeflood all conversations are resolved

Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for your contribution and welcome to the community !

@seeflood seeflood merged commit 57d44ac into mosn:main May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mix pubsub demo
3 participants