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

Add devcontainer for ACA-Py #2267

Merged

Conversation

usingtechnology
Copy link
Contributor

Addressing an issue where it is difficult to get set up to debug and begin coding by adding a devcontainer.

See Issue #2251 - this is only one small part of developer documentation. I wanted this out for feedback while we identify other areas of concern.

Addressing an issue where it is difficult to get set up to debug and begin coding by adding a devcontainer.

Signed-off-by: Jason Sherman <[email protected]>
@swcurran swcurran requested a review from amanji June 15, 2023 13:54
@swcurran
Copy link
Contributor

@amanji please check this out. @dbluhm, is there anyone on your team that regularly use Dev Containers?

@amanji
Copy link
Contributor

amanji commented Jun 15, 2023

So far so good, I was able to load the devcontainer in VSCode without any issues.

Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

These are great! couple small comments
I started from scratch, didn't even have the dev container extension installed and can get things going with the instructions and container config here

devcontainer.md Show resolved Hide resolved
devcontainer.md Show resolved Hide resolved
devcontainer.md Outdated Show resolved Hide resolved
@swcurran
Copy link
Contributor

I would add a note of what you can't do with a dev container -- notably running the various Docker-based tools in the repo (e.g. run_docker in the scripts folder, the run_demo in the demo/ folder. The devcontainer isn't setup to do docker-in-docker, so those can't be used. Not sure if they should be able to be used, so not a big issue, but should be mentioned since we lean heavily on those tools.

Looks great and after the first build is pretty fast!

@swcurran
Copy link
Contributor

Something that might be worth trying. Evidently, docker-in-docker is possible with devcontainers, as per this: https://github.com/devcontainers/features/tree/main/src/docker-in-docker. It might be interesting to see if with that added to the devcontainer config if the ACA-Py demos can be run.

@usingtechnology
Copy link
Contributor Author

Yes, I took a quick look at docker-in-docker, but didn't explore it fully (or try to implement it). I could spend more time on it if we want.

@swcurran
Copy link
Contributor

How about for now, we just add a note about it in the readme, and an issue with a “help wanted” sign.

@dbluhm
Copy link
Contributor

dbluhm commented Jun 27, 2023

I don't have anyone on my team actively using devcontainers right now but I think this would be a great addition. I agree with Stephen on getting this merged sooner than later and plan on making further enhancements in the future.

@usingtechnology usingtechnology marked this pull request as ready for review July 3, 2023 18:32
dbluhm
dbluhm previously approved these changes Jul 4, 2023
@usingtechnology
Copy link
Contributor Author

added docker-in-docker and updated the devcontainer readme to show how to run the demos within the container.

@WadeBarnes
Copy link
Contributor

For the devContainer, I'd recommend using bullseye rather than buster to keeps things consistent with the published images.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@usingtechnology
Copy link
Contributor Author

For the devContainer, I'd recommend using bullseye rather than buster to keeps things consistent with the published images.

Thanks for that... updated

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Tested out with running the demo and worked nicely. A couple of notes we can add, but overall looks good. Great work!

@swcurran swcurran merged commit 074243a into openwallet-foundation:main Jul 5, 2023
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.

6 participants