-
Notifications
You must be signed in to change notification settings - Fork 12
Added Dockerfile with Bedrock prerequisites #1407
base: master
Are you sure you want to change the base?
Conversation
@andrebriggs - there was a plan to follow up on this? |
Hi @carlessanagustin thanks for the PR! Typically when making a PR for something more substantial we ask folks follow this guidance. I've added some feedback to the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,3 +1,6 @@ | |||
from: https://youtu.be/H5uq60pb2Hs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove
@@ -0,0 +1,20 @@ | |||
FROM debian:stretch | |||
LABEL createdBy="https://twitter.com/carlesanagustin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove
then | ||
exit 1 | ||
fi | ||
# next 6 lines commented due to automation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these changes to the shell scripts make it difficult to re-use as they previously were. I suggest adding an override instead of commenting out these blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting an environment variable in the Dockerfile like ASSUME_YES
or NO_INPUT
, maybe prefixed with BEDROCK_
could work?
Added Dockerfile with Bedrock prerequisites installed in Debian based image ready to use. Instructions and usage come in Makefile. Documentation is updated in 2x README.md files.