ctubbsii commented on code in PR #384:
URL: https://github.com/apache/accumulo-website/pull/384#discussion_r1175800039
##########
Dockerfile:
##########
Review Comment:
I think this Dockerfile could use some comments to explain its intended
purpose as a short summary.
##########
build-images.sh:
##########
@@ -0,0 +1,12 @@
+#!/bin/bash
+set -e
+
+# This script is used to build and tag both docker images while still
+# keeping the images in the same Dockerfile.
+# Doing this ensures that both images will have synced dependencies.
+
+BASE_TAG="webdev"
+VALIDATE_TAG="${BASE_TAG}-validator"
+
+docker build --target base -t ${BASE_TAG} .
+docker build -t ${VALIDATE_TAG} .
Review Comment:
Quotes are needed here. Curly braces aren't.
```suggestion
docker build --target base -t "$BASE_TAG" .
docker build -t "$VALIDATE_TAG" .
```
##########
build-images.sh:
##########
Review Comment:
This file could be put in the `_scripts/` directory and the first line could
be modified to conform to the same standard `#! /usr/bin/env bash`. I'm not
sure if there's a better place for the Dockerfile, too, but I do know we have a
lot of clutter at the root, so if there is a possibility of a better location,
that might be good too. If there isn't a better location, I think the
Dockerfile can stay at the root, but I think this could still be moved to the
`_scripts/` directory. It would also be good to use `shellcheck` to test the
script for quality, and maybe even `shfmt` (maybe `shfmt -ln bash -l -d -i 2
-ci -s .` like the main accumulo repo) to verify some standardized formatting
(but this is not strictly necessary... especially since this is a very small
script).
##########
Dockerfile:
##########
@@ -0,0 +1,30 @@
+FROM ruby:2.7.8-slim-bullseye as base
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ build-essential \
+ git \
+ curl \
+ && rm -rf /var/lib/apt/lists/*
+
+WORKDIR /site
+
+COPY Gemfile /site/Gemfile
+COPY Gemfile.lock /site/Gemfile.lock
Review Comment:
Is there a one-line comment that could be added to explain why these are
being copied? It's not clear to me, since this is not a normal step when
building locally.
##########
README.md:
##########
@@ -108,6 +108,69 @@ HTML styled "just right".
Jekyll will print a local URL where the site can be viewed (usually,
[http://0.0.0.0:4000/](http://0.0.0.0:4000/)).
+### Testing using Docker environment
+
+#### Build environment
+A containerized development environment can be built using the local
+Dockerfile.\
Review Comment:
I see you're using a lot of backslashes here in the README to add arbitrary
line breaks. Micro-managing the flow like that is a very different style of
writing from our usual Markdown. I recommend just sticking to regular paragraph
flow, and let the browser handle line wrapping automatically for long lines. If
a sentence needs to stand out on its own, rather than just using a line break,
just put a blank line between to make a new new paragraph. My understanding is
that separate paragraphs are also better for some screen readers for people
with low vision, as well, but I could be wrong about that. At the very least,
that would be more consistent with our existing style, and a lot easier to
maintain.
```suggestion
Dockerfile.
```
##########
Dockerfile:
##########
@@ -0,0 +1,30 @@
+FROM ruby:2.7.8-slim-bullseye as base
+
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ build-essential \
+ git \
+ curl \
+ && rm -rf /var/lib/apt/lists/*
+
+WORKDIR /site
+
+COPY Gemfile /site/Gemfile
+COPY Gemfile.lock /site/Gemfile.lock
+
+RUN gem update --system && bundle install && gem cleanup
Review Comment:
I'm not super familiar with the base ruby image, but in my environment, the
system gems are provided by system RPMs in my distribution. This has been my
experience using CentOS/RHEL/Fedora. So, a `yum` (or `dnf`) update is normally
sufficient and I don't need to do a `gem update`. Is the situation different
for the base ruby image?
Also, the instructions on the README in
https://github.com/apache/accumulo-website#local-builds-for-testing explain in
detail how to set up the GEM environment as a non-supervisor user for local
gems installed to the user directory by bundler. I think it would be helpful if
this Dockerfile did something similar, just automated, so instead of setting
things up differently by doing system installed gems. That way, there's only a
single set of instructions to follow, whether we're doing something via a local
build, or trying to maintain this Dockerfile in future. However, it looks like
this is relying on a different set of environmental assumptions from the those
in the README. It looks like this is installing gems using the superuser, and
gems are being installed to a system directory rather than a local user
directory. I'm not 100% certain of this, since I'm not super familiar with
Docker. If this is diverging from the detailed README instructions, it would be
helpful to have some
inline comments explaining what's being done here, so we have a better chance
of being able to maintain it going forward.
##########
build-images.sh:
##########
@@ -0,0 +1,12 @@
+#!/bin/bash
+set -e
+
+# This script is used to build and tag both docker images while still
+# keeping the images in the same Dockerfile.
+# Doing this ensures that both images will have synced dependencies.
+
+BASE_TAG="webdev"
+VALIDATE_TAG="${BASE_TAG}-validator"
Review Comment:
Curly braces aren't needed here, but it's not hurting, so it's up to you.
```suggestion
VALIDATE_TAG="$BASE_TAG-validator"
```
##########
README.md:
##########
@@ -108,6 +108,69 @@ HTML styled "just right".
Jekyll will print a local URL where the site can be viewed (usually,
[http://0.0.0.0:4000/](http://0.0.0.0:4000/)).
+### Testing using Docker environment
+
+#### Build environment
+A containerized development environment can be built using the local
+Dockerfile.\
+Run the build-images.sh script to generate the development environment and
+associated images.
+
+```bash
+./build-images.sh
+```
+
+This action will produce two containers: `webdev` and `webdev-validator`.\
+The webdev container will execute a `jekyll serve` command with the
+polling option enabled.\
+This provides the ability to immediately review rendered content changes.
Review Comment:
```suggestion
This action will produce two containers: `webdev` and `webdev-validator`.
The webdev container will execute a `jekyll serve` command with the
polling option enabled.
This provides the ability to immediately review rendered content changes.
```
##########
README.md:
##########
@@ -108,6 +108,69 @@ HTML styled "just right".
Jekyll will print a local URL where the site can be viewed (usually,
[http://0.0.0.0:4000/](http://0.0.0.0:4000/)).
+### Testing using Docker environment
+
+#### Build environment
+A containerized development environment can be built using the local
+Dockerfile.\
+Run the build-images.sh script to generate the development environment and
+associated images.
+
+```bash
+./build-images.sh
+```
+
+This action will produce two containers: `webdev` and `webdev-validator`.\
+The webdev container will execute a `jekyll serve` command with the
+polling option enabled.\
+This provides the ability to immediately review rendered content changes.
+
+```bash
+docker run -d -v "$PWD":/site -p 4000:4000 webdev
+```
+
+Shell access can be obtained by overriding the default container command.\
+This is useful for adding new gems, or modifying the Gemfile.lock for updating
+existing dependencies.
Review Comment:
```suggestion
Shell access can be obtained by overriding the default container command.
This is useful for adding new gems, or modifying the Gemfile.lock for
updating
existing dependencies.
```
##########
README.md:
##########
@@ -108,6 +108,69 @@ HTML styled "just right".
Jekyll will print a local URL where the site can be viewed (usually,
[http://0.0.0.0:4000/](http://0.0.0.0:4000/)).
+### Testing using Docker environment
+
+#### Build environment
+A containerized development environment can be built using the local
+Dockerfile.\
+Run the build-images.sh script to generate the development environment and
+associated images.
+
+```bash
+./build-images.sh
+```
+
+This action will produce two containers: `webdev` and `webdev-validator`.\
+The webdev container will execute a `jekyll serve` command with the
+polling option enabled.\
+This provides the ability to immediately review rendered content changes.
+
+```bash
+docker run -d -v "$PWD":/site -p 4000:4000 webdev
+```
+
+Shell access can be obtained by overriding the default container command.\
+This is useful for adding new gems, or modifying the Gemfile.lock for updating
+existing dependencies.
+
+```bash
+docker run -v "$PWD":/site -it webdev /bin/bash
+```
+
+Mounting the local directory as a volume is recommended to ensure that Gemfile
and
+Gemfile.lock stay updated with any dependency changes.
+
+#### Validation environment
+
+The `webdev-validator` image can be used to run validation tests against the
+rendered website content.\
+The output directory `_site` needs to be volume mounted for these tests to
work.
Review Comment:
```suggestion
The `webdev-validator` image can be used to run validation tests against the
rendered website content.
The output directory `_site` needs to be volume mounted for these tests to
work.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
