[GitHub] [accumulo-website] ctubbsii commented on a diff in pull request #384: Add containerized development environment

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #384:
URL: https://github.com/apache/accumulo-website/pull/384#discussion_r1172024158


##########
README.md:
##########
@@ -108,6 +108,31 @@ 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 
+
+A containerized development environment can be built using the local Dockerfile. 
+
+```bash
+docker build -t webdev .
+```
+
+The webdev container's run command will run jekyll's serve command with the 
+polling option enabled.  This allows for immediate review of rendered changes.   

Review Comment:
   Consecutive spaces get collapsed in rendered Markdown/HTML, so the double space after a period can be removed. Trailing spaces throughout our docs should also be removed. I've suggested changes to this one line, but it should be addressed everywhere in new PRs.
   
   ```suggestion
   polling option enabled. This allows for immediate review of rendered changes.
   ```



##########
Gemfile:
##########
@@ -2,5 +2,6 @@ ruby '>=2.5.1'
 source 'https://rubygems.org'
 gem 'jekyll', '>= 4.2.0'
 gem 'jekyll-redirect-from', '>= 0.16.0'
+gem 'html-proofer', '>= 4.4.0'

Review Comment:
   A comment should be added here, to note that html-proofer is not used for the build, but just specified so the optional link checking step can be manually executed.



##########
README.md:
##########
@@ -108,6 +108,31 @@ 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 
+
+A containerized development environment can be built using the local Dockerfile. 
+
+```bash
+docker build -t webdev .
+```
+
+The webdev container's run command will run jekyll's serve command with the 
+polling option enabled.  This allows for immediate review of rendered changes.   
+
+```bash
+docker run -d -v $PWD:/site -p 4000:4000 --name accumulo-website webdev

Review Comment:
   I'm not sure if `$PWD` is guaranteed to be set or if it always returns what you want it to (it can contain relative paths, I think)... but it definitely could contain spaces, so quoting it is necessary. I'm making this suggestion once, but it should be fixed everywhere.
   
   ```suggestion
   docker run -d -v "$PWD":/site -p 4000:4000 --name accumulo-website webdev
   ```



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org