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

Various README improvements #585

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Various README improvements #585

wants to merge 6 commits into from

Conversation

PeterJCLaw
Copy link
Member

@PeterJCLaw PeterJCLaw commented Mar 2, 2024

This aims to make the process easier for people not already familiar with the setup, primarily by guiding them through the preferred (local install) path which has each of the necessary steps laid out. It also explains how to run the spell checking locally.

Finally it calls out the security risks associated with the Docker path, removes the instructions there to reduce the chances that people not equipped to judge those risks take that path and implies (correctly) that the spell checking is not supported under Docker.

Once we're happy with this I'll replay these changes against our other Jekyll based sites.

@WillB97
Copy link
Contributor

WillB97 commented Mar 3, 2024

Is the local setup actually the preferred method? The primary change here seems to be about changing the preference from docker to local.

Docker has the advantage that it doesn't require users to install various additional software (node, npm, rake) just to update the content. This is made more notable by the old version of node and rake needed in this repository.

@PeterJCLaw
Copy link
Member Author

Is the local setup actually the preferred method? The primary change here seems to be about changing the preference from docker to local.

Yes, local setup is the method which is common to all our Jekyll sites. The Docker setup in this repo is an exception to that and one which was poorly documented. Note that e.g: the instructions for the Docker setup didn't previously note the need to clone the repo nor provide any information on how to set up or use Docker.

Since any non-trivial operations in this repo require the developer to understand the Ruby/Jekyll/Liquid setup to at least a basic level, we need to provide those instructions anyway. Any support for Docker is then a strict increase in the maintenance burden.

Docker has the advantage that it doesn't require users to install various additional software (node, npm, rake) just to update the content. This is made more notable by the old version of node and rake needed in this repository.

Using Docker is not better it is just different. It's not always easier to install nor is it even fewer things to install. Installing Docker is something which requires the user to have local admin on their computer, as well as having an awareness of the security impacts of installing a container engine. The latter is especially pertinent given the current security issues with the Docker based setup here.

As noted previously -- since we need to support the Ruby route anyway, any issues which exist with that need fixing regardless of whether we also support the Docker route.

Please also bear in mind that whatever is done for this repo needs replicating for all our other Jekyll repos (of which there are four), so the incremental effort for having an extra setup method is not trivial.

Finally, as noted in other issues on this repo (e.g: #488) the current Docker setup is somewhat out of sync with the Ruby setup -- meaning that users still need to install the other dependencies anyway. I'd much rather put the effort into having a single approach we can make work well rather than two approaches which work badly.

@PeterJCLaw PeterJCLaw mentioned this pull request Mar 6, 2024
Importantly this pushes this option to being secondary and
clarifies which of the build steps it replaces.

Note that this also removes the instructions on how to use Docker.
If users are not already familiar with Docker & Docker Compose
then they are equally not equipped to judge the security posture
that it presents.
@PeterJCLaw
Copy link
Member Author

Rebased to fix lots of conflicts.

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