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

hello-world - add folder paths to launch for split projects #493

Merged
merged 2 commits into from
May 23, 2022

Conversation

tonysurma
Copy link
Contributor

Description

In the hello-world quickstart, I added folder paths to launch.json for split projects. The combination of adding in the split project folders for node and python as well as the addition of the VS Code debugging configurations are out of synch and corrected by adding the additional folder paths in launch.json for the hello-world quickstart.

Issue reference

closes #492

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

wcs1only
wcs1only previously approved these changes Nov 17, 2021
greenie-msft
greenie-msft previously approved these changes Feb 15, 2022
@paulyuk
Copy link
Contributor

paulyuk commented Feb 25, 2022

Hi @tonysurma, I unblocked the tests so they can run. Could you please ensure they pass, and also click on the DCO workflow step above and sign the DCO? you can alternatively use a git commit -s -p in the future to sign your commit and see the changes before pushing.

@paulyuk
Copy link
Contributor

paulyuk commented Feb 25, 2022

BTW nice catch and thank you much for the contribution

@paulyuk
Copy link
Contributor

paulyuk commented Feb 25, 2022

Hi Tony, i see the tests failed based on a test bug we fixed in master branch. Please sync your branch and fork with remote master, merge and push. You'll get updated readme.md files with fixed tests, and all should work dandy after that.

Here is the test bug that got fixed: #548

@tonysurma
Copy link
Contributor Author

thank you both, will do that soon and glad this contribution can make it in

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.

Just flipping to request changes until you can merge the test fix and resubmit. this way I know we're waiting on something.

@tonysurma
Copy link
Contributor Author

rebased with DCO so should be good to go - thanks again

@tonysurma
Copy link
Contributor Author

rebased again and requested review, thanks again

@yaron2
Copy link
Member

yaron2 commented Apr 26, 2022

@tonysurma apologies it has taken so long to merge. please resolve the conflicts one last time and we'll merge :)

@amulyavarote
Copy link
Contributor

Do we still need this based on our new quickstarts structure?

@amulyavarote
Copy link
Contributor

@tonysurma Can you please take the recent changes from master and rebase this with master and push the changes?

@tonysurma
Copy link
Contributor Author

Yes, will do as soon as I can

@tonysurma
Copy link
Contributor Author

before I do I wanted to verify the following approach given the changes. I see that hello-world has moved under tutorials so will do the update there. However I also see that the whole .vscode directory that was under the hello-world tutorial is now removed. To reapply this change I would need to recreate the .vscode directory and the two files that were there prior and add this path update.

Do you agree with this approach? Thanks.

@amulyavarote
Copy link
Contributor

before I do I wanted to verify the following approach given the changes. I see that hello-world has moved under tutorials so will do the update there. However I also see that the whole .vscode directory that was under the hello-world tutorial is now removed. To reapply this change I would need to recreate the .vscode directory and the two files that were there prior and add this path update.

Do you agree with this approach? Thanks.

Hi! Yea It looks good to me. Please push the changes to the PR. We will talk a look.

@amulyavarote
Copy link
Contributor

@tonysurma Do you have any updates on this PR?

@msfussell
Copy link
Member

@tonysurma - Ping?

Signed-off-by: tonysurma <5067595+tonysurma@users.noreply.github.com>
@tonysurma
Copy link
Contributor Author

apologies @msfussell and @amulyavarote for delay. I realized I had to recreate the whole branch to 'move' over to the new structure etc. and wanted to do it in this PR branch. Some git commands later and should be good to go.

@amulyavarote
Copy link
Contributor

@tonysurma Can you please rebase the changes with master and push the changes?

@tonysurma
Copy link
Contributor Author

Done, thx

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 71e5489 into dapr:master May 23, 2022
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-world - conflict in VS Code debug config and split projects
7 participants