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

clarify start up instructions for those completely new to docker #69

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

dcconner
Copy link
Contributor

Added a bit more step-by-step instructions on docker start up based on question from tester on new computer.

For docker compose run base bash is the bash necessary?

I've only done docker compose run base in new terminals and it seemed to work fine.

@dcconner dcconner requested review from sea-bass and ct2034 October 15, 2024 18:58
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Thank you!

To answer your question, I think the bash is not needed since I restored the entrypoint in #66 -- but, see my inline comment for what it should be!

README.md Outdated
Comment on lines 85 to 86
```bash
docker compose run base bash
Copy link
Contributor

@sea-bass sea-bass Oct 16, 2024

Choose a reason for hiding this comment

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

I didn't notice this before, but doing docker compose run base, this actually creates a new container instead of attaching to the one that was just started.

So we actually want to do this to attach to the existing container.

docker compose exec base bash

You can verify the difference by typing docker ps.

@dcconner
Copy link
Contributor Author

@ct2034 if you approve, feel free to squash and merge

@dcconner
Copy link
Contributor Author

Modified p3 world to match description. I have NOT tested this with other technologies to make sure it did not break their demonstrations. Either we need this change, or change the description in README to be consistent

@sea-bass
Copy link
Contributor

I'd say just merge this PR, but maybe explicitly send an email about the modified world file since everyone will need to tweak the solutions.

@dcconner dcconner merged commit 3166770 into main Oct 17, 2024
2 checks passed
@dcconner dcconner deleted the clarify-startup branch October 17, 2024 00:30
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.

2 participants