Skip to content

Conversation

0xlynett
Copy link
Contributor

@0xlynett 0xlynett commented Apr 4, 2024

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

Style comment: please add some newlines to follow our convention having one sentence per line to make maintenance easier (e.g. prose diffs in PRs are much more readable w/ one sentence per line).

I want to add some discussion of common permutations users who are less familiar with Docker will probably want to know how to do, e.g.

  1. Run multiple nodes (i.e. how to do port mapping properly & how to set up/use multiple volumes),
  2. Copying native home dir in/out of volumes.

I'm sure there will be others. Anyways, you don't have to add this here if you don't want; I can follow up and add it.

The other thing I want to wait on before merging is testing on MacOS. I'm updating a machine now to test on. Then we can decide whether to "fully recommend" Docker or whether we should still mainly recommend using native binaries with Docker as a secondary suggestion.

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

Overall comment: please add newlines between sentences so that each sentence is on its own line.

Other than that, I left some more specific comments. Please address comments if they are all clear and then merge when ready.

Thanks for the PR! 🚀

@dr-frmr
Copy link
Contributor

dr-frmr commented Apr 5, 2024

Can merge after 1-sentence-per-line is done

@0xlynett
Copy link
Contributor Author

0xlynett commented Apr 5, 2024

added all the stuff you mentioned except copying native home dir in/out of a volume, due to my lack of experience with docker i'm not sure how to do that properly haha

@nick1udwig
Copy link
Member

added all the stuff you mentioned except copying native home dir in/out of a volume, due to my lack of experience with docker i'm not sure how to do that properly haha

I can handle that, thanks for the changes!

@nick1udwig nick1udwig merged commit a5800ea into hyperware-ai:main Apr 5, 2024
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