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

contrib: Finish Docker documentation. #3045

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Jan 13, 2023

This is rebased on #3044.

This updates the contrib/docker/README.md and contrib/docker/Dockerfile to address the remaining outstanding TODOs and finish fleshing out the documentation.

@davecgh davecgh added the documentation Issues and/or pull requests related to documentation. label Jan 13, 2023
@davecgh davecgh added this to the 1.8.0 milestone Jan 13, 2023
@davecgh davecgh force-pushed the contrib_docker_docs branch 2 times, most recently from ee47492 to 5b1f1cc Compare January 14, 2023 04:18
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Overall the documentation looks fine. Theres a bit too much repetition for my taste, e.g. the concept of running as 10000:10000 is explained in detail twice, the dcrctl RPC examples are listed twice, including the caveat about dcrwallet not being installed. I understand the intention is to have a quick summary and a more in-depth explanation, but perhaps some of the duplication can be removed by using links.

Defaulting to always build latest master seems a bit off to me. I believe most users would want to be running a release version and defaulting to the latest release would be more sensible. I notice you've listed that as a TODO though, so I guess that can be left for now.

contrib/docker/Dockerfile Show resolved Hide resolved
contrib/docker/Dockerfile Show resolved Hide resolved
contrib/docker/README.md Outdated Show resolved Hide resolved
contrib/docker/entrypoint/entrypoint.go Outdated Show resolved Hide resolved
@davecgh davecgh force-pushed the contrib_docker_docs branch 2 times, most recently from f2d9d47 to 9da601a Compare January 17, 2023 15:24
@davecgh
Copy link
Member Author

davecgh commented Jan 17, 2023

Thanks for the review!

...the concept of running as 10000:10000 is explained in detail twice,

Good point. Updated to put a link in the overview to the section discussion this to avoid that repetition.

the dcrctl RPC examples are listed twice...

I believe they are all different with the exception of running the container where the important aspect about the DNS names are explained in the RPC Server Authentication section. As you mentioned that is so there can be a quick start section and then the reader can find out the details of why.

I suppose the RPC Server Authentication could link to the Quick Start section for the example, but I personally find that style where I have to jump around a lot annoying as a reader when I'm first reading about a new system. I'm not fundamentally opposed to changing that if others prefer jumping around instead though.

Defaulting to always build latest master seems a bit off to me. I believe most users would want to be running a release version and defaulting to the latest release would be more sensible. I notice you've listed that as a TODO though, so I guess that can be left for now.

Yeah, I wasn't 100% sure on that point since I don't really use docker and why I requested a review from someone who does. I would imagine that it will highly depend on the user which is why I added the TODO since I figured having a choice would cover the various use cases. For example, I pretty much always run master on all of the core Decred software, so being unable to do that would be one more reason not to use Docker.

Another thing to keep in mind about building a release is that they typically are fairly tightly coupled to the Go version that was the latest at the time of the release, so I suspect that having a release version would really end up needing to have a directory per release version that pulls the specific Go version to build with too.

Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Updates look good.

I guess which version to build and which go version to use really depends on the target audience. Devs probably want latest, people deploying for production probably want release. We can maybe address both just by using params (default to release because devs by their nature are more comfortable changing things via params). That should satisfy devs and also make deployment onto things like Umbrel/Unraid/Proxmox/etc nice and simple.

This updates the contrib/docker/README.md and contrib/docker/Dockerfile
to address the remaining outstanding TODOs and finish fleshing out the
documentation.
@davecgh davecgh merged commit a3064c1 into decred:master Jan 19, 2023
@davecgh davecgh deleted the contrib_docker_docs branch January 19, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues and/or pull requests related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants