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
Container improvements #540
base: main
Are you sure you want to change the base?
Conversation
c93b6d7
to
98e2c32
Compare
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.
Could we also update Config and Docker section in README.md please?
Dockerfile
Outdated
@@ -1,5 +1,9 @@ | |||
FROM python:3-slim | |||
|
|||
ENV IN_CONTAINER=true | |||
ENV OS_OUTSIDE_CONTAINER="Linux/Red Hat Enterprise Linux 8.8 (Ootpa)" |
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 should be an argument, so user can setup the image with specific OS and SHELL names.
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.
In the upcoming commit I'll leave it as ENV variable that is in the config.py DEFAULT_CONFIG.
Reason being that it might be handy to leave it interchangeable, e.g.:
- All of a sudden you need to work on a server with an os / shell you're very unfamiliar with. You can temporarily overwrite your os and shell name of your often used shellgpt to mock this environment, without having to install on that server / install in a similar virtual local environment
- Similar to above, if I'd like to use shellgpt containers for multiple setups: RHEL, Debian, Ubuntu, powershell, MacOs, ... I can run the same image each time, as long as I overwrite the OS and shell to mock these envs. If I had to make a different image each time that'd be a waste of diskspace and image build time in my opinion.
These settings worked for me:
|
@@ -57,7 +57,7 @@ def main( | |||
rich_help_panel="Assistance Options", | |||
), | |||
interaction: bool = typer.Option( | |||
True, | |||
True if cfg.get("SHELL_INTERACTION") != "false" else False, |
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.
True if cfg.get("SHELL_INTERACTION") != "false" else False, | |
cfg.get("SHELL_INTERACTION") == "true", |
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.
I wrote it this way to ensure the normal default behaviour is set, also when SHELL_INTERACTION contains an unintended / weird value
Dockerfile
Outdated
ENV SHELL_INTERACTION=false | ||
ENV OVERWRITE_OS_NAME="" | ||
ENV OVERWRITE_SHELL_NAME="" | ||
|
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.
ENV SHELL_INTERACTION=false | |
ENV OVERWRITE_OS_NAME="" | |
ENV OVERWRITE_SHELL_NAME="" | |
ARG OVERWRITE_OS_NAME=default | |
ARG OVERWRITE_SHELL_NAME=default | |
ENV SHELL_INTERACTION=false | |
ENV OVERWRITE_OS_NAME=${OVERWRITE_OS_NAME} | |
ENV OVERWRITE_SHELL_NAME=${OVERWRITE_SHELL_NAME} | |
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.
I believe you misunderstand Docker's ARG:
These are env variables that only exist during the building process of the container image.
Once the image exists, these are gone forever.
Please refer the dockerfile arg documentation for a better, more complete explanation.
ENV sets environment variables that remain, and can be used set as arguments with --env as desired when starting the container
default OVERWRITE_OS_NAME value "default" + got rid of redundant else Co-authored-by: Farkhod Sadykov <[email protected]>
OVERWRITE_SHELL_NAME default value "default" + got rid of redundant else Co-authored-by: Farkhod Sadykov <[email protected]>
Co-authored-by: Farkhod Sadykov <[email protected]>
IN_CONTAINER: setting this one to true, helps fix some default things that aren't handy when in a container: