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

Improve dev container support #5

Merged
merged 32 commits into from
May 31, 2022
Merged

Improve dev container support #5

merged 32 commits into from
May 31, 2022

Conversation

SergioGasquez
Copy link
Member

  • Add devcontainer support for:
    • VS Code Dev Containers
    • Gitpod
    • GitHub Codespaces
  • When using dev containers:
    • Wokwi support
    • Web flasher support
    • Readme with instructions

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

This is awesome! LGTM!

I did have a question about the two new shell scripts, are they needed for use in Rust projects like this one? Seems like the only thing that needs to be set is WOKWI_HOST, then it's possible to use web-flash or wokwi-server as a cargo runner? Just a thought! By no means something to stop this being merged :).

@SergioGasquez
Copy link
Member Author

Thanks for the quick review! The main purpose of the scripts is to make VS tasks less complex and, when using Wokwi in Gitpod/GH Codespaces, remove the need to export WOKWI_HOST from the user.

That being said, we could maybe move the complexity away from the scripts into wokwi-server and task.json and get rid of the 3 bash files

@jessebraham
Copy link
Member

Thank you for this Sergio, great work!

I would personally like to see the scripts either moved into a scripts directory or (preferably) removed altogether if possible. Again though, this is not a blocking issue and I'm more than happy to merge this as-is. I completely understand if you've moved on to other things and don't want to poke at this anymore right now, so please just let me know your plan and I will act accordinly.

@SergioGasquez
Copy link
Member Author

I moved the scripts into a scripts folder for the moment and in the future, I will be trying to get rid of them by moving part of the logic to wokwi-server

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

@jessebraham jessebraham merged commit 456edf2 into esp-rs:main May 31, 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.

3 participants