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

Update README.md #963

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Update README.md #963

merged 3 commits into from
Feb 29, 2024

Conversation

farshaddavoudi
Copy link
Contributor

@farshaddavoudi farshaddavoudi commented Jan 5, 2024

Fix typos, problematic symbols and format, and some adjusting

Description

Please explain the changes you've made

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: #[issue number]

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.

Fix typos, problematic symbols and format, and some adjusting

Signed-off-by: Farshad Davoudi <f.davoudi.r@outlook.com>
@paulyuk paulyuk added this to the 1.13 milestone Feb 11, 2024
Copy link
Contributor

@paulyuk paulyuk left a comment

Choose a reason for hiding this comment

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

Please see one suggested tweak.

Also is there any change you could target this PR at the release-1.13 branch instead of master?

state_management/csharp/sdk/README.md Outdated Show resolved Hide resolved
state_management/csharp/sdk/README.md Outdated Show resolved Hide resolved
@farshaddavoudi farshaddavoudi changed the base branch from master to release-1.13 February 14, 2024 17:28
Signed-off-by: Farshad Davoudi <f.davoudi.r@outlook.com>
@paulyuk
Copy link
Contributor

paulyuk commented Feb 21, 2024

Hi @farshaddavoudi - something is wrong with the state management quickstart, so it won't pass checks anymore.

what i usually do is cd into that quickstart folder and do a make validate on my local machine to see if test will pass, or why not. do you have time to try?

@farshaddavoudi
Copy link
Contributor Author

Hi @farshaddavoudi - something is wrong with the state management quickstart, so it won't pass checks anymore.

what i usually do is cd into that quickstart folder and do a make validate on my local machine to see if test will pass, or why not. do you have time to try?

Hi @paulyuk
I installed the Make utility and tried to figure out what's the root of the problem. As I perceived, the error message which a screenshot is attached, indicates that the make command is unable to execute the mm.py script, because it cannot find it. I searched / scanned for the script in the entire quickstart project without any luck. I don't know if I'm missing something or the script is erased somehow.

image

@paulyuk
Copy link
Contributor

paulyuk commented Feb 23, 2024

Hi @farshaddavoudi - something is wrong with the state management quickstart, so it won't pass checks anymore.
what i usually do is cd into that quickstart folder and do a make validate on my local machine to see if test will pass, or why not. do you have time to try?

Hi @paulyuk I installed the Make utility and tried to figure out what's the root of the problem. As I perceived, the error message which a screenshot is attached, indicates that the make command is unable to execute the mm.py script, because it cannot find it. I searched / scanned for the script in the entire quickstart project without any luck. I don't know if I'm missing something or the script is erased somehow.

image

Hi @farshaddavoudi sorry i did not include details about this. To do the local test you need Mechanical Marchdown which is a small utility found here: https://github.com/dapr/mechanical-markdown

To install you just do this:
pip3 install mechanical-markdown

Then that command should work well in each quickstart folder that has a makefile.

Making some tweaks to STEPs so project can build and tests pass.

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

```bash
dotnet restore
Copy link
Contributor

Choose a reason for hiding this comment

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

The test was failing because the dotnet build was not happening in order-processor subfolder. You could either set the working directory or cd into it.

However your simplification of restore and build made me think we should just further simplify. Dotnet language tools do a restore and build on run. So let's go straight to the run, which is done as a part of dapr run -f ..

@@ -70,6 +58,7 @@ sleep: 15
cd ./order-processor
dapr run --app-id order-processor --resources-path ../../../resources/ -- dotnet run
```
<!-- END_STEP -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This end step was missing, which was problematic for test framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @paulyuk, I apologize for not being able to participate and complete the job. I was busy preparing for my Azure certification exam. Thank you for approving my work.

Copy link
Contributor

@paulyuk paulyuk left a comment

Choose a reason for hiding this comment

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

LGTM

@paulyuk paulyuk merged commit 9897c5c into dapr:release-1.13 Feb 29, 2024
6 of 7 checks passed
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.

2 participants