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 readme #481

Merged
merged 31 commits into from
Oct 3, 2023
Merged

Add devcontainer readme #481

merged 31 commits into from
Oct 3, 2023

Conversation

jrhemstad
Copy link
Collaborator

Description

As a precursor to adding a full-fledged contributor guide, this PR adds documentation about how to use our development containers.

It provides detailed instructions on both using the containers with vscode as well as using the docker images manually.

I updated the launch.sh script to add a --docker flag to simplify launching the docker container.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jrhemstad jrhemstad requested review from a team as code owners September 25, 2023 18:40
@jrhemstad jrhemstad requested review from jarmak-nv and ericniebler and removed request for a team September 25, 2023 18:40
Copy link
Contributor

@ahendriksen ahendriksen left a comment

Choose a reason for hiding this comment

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

This already looks really polished! I walked through the non-VS Code parts and only have one comment. Looking forward to see the documentation on how to run the tests.

.devcontainer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Left some minor comments, but generally LGTM. I would suggest adding a few more links to show people how they could customize devcontainers if they want to add specific features to help their development (e.g. installing their favorite non-VSCode text editor or other productivity tools for navigating files etc inside the container).

.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Nice job. I have a few suggestions, but approve once others think it's ready to go.

.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
@jrhemstad jrhemstad mentioned this pull request Sep 27, 2023
.devcontainer/launch.sh Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

LGTM

.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
.devcontainer/README.md Outdated Show resolved Hide resolved
Co-authored-by: Christopher Harris <[email protected]>
@jrhemstad jrhemstad merged commit af59bb6 into NVIDIA:main Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants