-
Notifications
You must be signed in to change notification settings - Fork 75
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
Added bashrc_extesions extension #296
base: main
Are you sure you want to change the base?
Conversation
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.
Looking at this we also need tests to consider merging it.
I'm a bit wondering if this is actually specific to the .bashrc or if it should simply be a multi argument that takes one filename in the container and then appends the contents of any of the following local paths to that filename with an append operator.
ala --append-file ~/.bashrc my_extension1 my_extension2 --
files[self.get_filename(bashrc_extension)] = f.read() | ||
elif bashrc_extension.startswith(('http://', 'https://')): | ||
try: | ||
response = urllib3.PoolManager().request('GET', bashrc_extension) |
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.
This is a big change in scope. Fetching remote content opens up a whole extra can of worms. You can easily wget it to file just before running with barely more characters. Lets not add this level of complexity. Especially as this is a path to full remote code execution in the environment when you enter the container as we're not validating the remote content.
WORKDIR @home_dir | ||
RUN echo "\n# Source custom bashrc extensions" >> @home_dir/.bashrc | ||
@[for path, filename in bashrc_extension_files.items()] | ||
COPY @path @filename |
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.
This is effectively a temporary file, it should probably go into somewhere like /tmp and not potentially override content in their home directory. (For example if someone tries to append their own local .bashrc
it might get messy.
@@ -0,0 +1,6 @@ | |||
WORKDIR @home_dir | |||
RUN echo "\n# Source custom bashrc extensions" >> @home_dir/.bashrc | |||
@[for path, filename in bashrc_extension_files.items()] |
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.
This is potentially adding a bunch of layers. It can at least collapse the RUN command into only one instance not one per file.
@@ -0,0 +1,6 @@ | |||
WORKDIR @home_dir |
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.
We need to be careful about setting this WORKDIR as it sideeffects downstream snippets.
This allows you to customize the .bashrc in any container. You can achieve something similar by using the
--volume ~/.bashrc
argument, but that's not always what you want. Sometimes you want to use the base image's native .bashrc and just extend it for your use case. You can specify a list of files/URLs of your favorite extensionsI often use
--bashrc-extensions https://gist.githubusercontent.com/agyoungs/64f19391f864d0ea082b8a4d87a90d2b/raw/.bash-prompt.sh
as that one is super helpful for me, but this extension is generic enough to allow anyone to customize it for their use case.bashrc extension gist-link