Fix 556

[macel94]

Description

No package-lock was needed because in the project yarn it's used, so i deleted it, generated the correct yarn.lock files.
I generated them after solving the audit issue about nth-check setting the version which yarn needs to solve in the "resolutions" part of the package.json files.

While recompiling with "npm run dev" i also noticed the package.json were missing nodemon in the "devDependencies" section so i fixed that too, and after another "yarn" i was able to run the solution successfully.

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: #556

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.

[macel94]

For the FE part that i changed, i re-tested locally and everything seems ok:
dapr init

cd to react-calculator

npm install
npm run buildclient

dapr run --app-id frontendapp --app-port 8080 --dapr-http-port 3507 node server.js

As specified in the tutorial here: https://github.com/MACEL94/fix-556/tree/fix-556/tutorials/distributed-calculator#running-the-quickstart-locally

[macel94]

@yaron2 let me know if I need to add any info or change something, thanks

[paulyuk]

Thank you much. I'm looking into why the first check failed, and once we rerun that we can merge this.

[macel94]

@yaron2 @paulyuk I believe this PR now also solves the failing tests problem, can i ask you to re-launch the tests? thank you

[macel94]

@paulyuk sorry to see there are still problems, my bad for not trying in the devcontainer before pushing(I didn't notice the .devcontainer folder).

Will try again locally as soon as I have time and get back to you

[macel94]

Hi @paulyuk @greenie-msft

I looked into it and we can add
"match_order: none" but that would not guarantee that the order specified(for example the 11) was correctly persisted(maybe another one was).

I'd suggest to add that but also to change the second log from

• "Successfully persisted state."
  to something with the id in it like
• "Successfully persisted state for Order ID: " + orderId

Because the important part here is just that the order was received and then correctly persisted, it doesn't matter if the log in the meantime do anything else.

Let me know your opinion, and if you agree I can pick this up and solve it.

[macel94]

Hi @paulyuk @greenie-msft @yaron2

if you're ok with the change please take this PR and re-publish the samples images, then i expect the test to pass.
It's not so straightforward for me to test the PR if i changed something in the code because the tests only use the published images on your official ghcr for how the pipelines are structured.

But i assume this is intended so let me know if you need me to test in any other way.

[greenie-msft]

@macel94 thanks for the contributions!

One idea that I had to test these changes before merging into master is to create a new branch (in the quickstart repo) and target this PR at that branch.

We would then add the new branch into this list that automatically publishes new images: https://github.com/dapr/quickstarts/blob/master/.github/workflows/build.yml#L18

That workflow ^ is what is responsible for pushing new images of the src code.

What do you think about that? Would that be an easy way to test your changes before merging into master? If so, I can create that new branch now based off of master.

[macel94]

@greenie-msft

yes that would help me!
No problem for me creating another PR to target the other branch :)
feature/556-tutorials-fix maybe?

Thank you

[greenie-msft]

@macel94 Branch is created and the workflow has been updated to run on changes to the branch. Please open the PR targeting https://github.com/dapr/quickstarts/tree/feature/556-tutorials-fix and let's see if the tests pass.

Thank you!

[msfussell]

@macel94 - Did you get the tests to run successfully?

[macel94]

Hi @msfussell I tagged you in #771 explaining why right now they can't pass. I changed part of the code that needs to be built and published before the tests can pass(as the tests use the latest image published, not the actual source code).

Before they can pass the other pr(#771) needs to be merged so that the new images are published with the ID in the log message when the order is processed.

Let me know if I didn't explain well what I meant

[paulyuk]

#771 is in, pending that we do this.

[paulyuk]

Hi @macel94 note we went in a direction to rewrite/refactor the entire suite of actions to reduce core parallelism and reliability issues. The work you've done here definitely contributed to and influenced the direction. As for this exact PR, it may or may not make sense to sync it and pursue it after we've done the extensive changes in #805 . Happy to chat.

[macel94]

Hi @paulyuk

this was primarily opened to fix the #556 issue, so i think could/should still be merged.
If you want me to undo the changes i made to the readme files instead I can, i understood they're not needed anymore.

[paulyuk]

LGTM - improves quality and nth check