-
Notifications
You must be signed in to change notification settings - Fork 887
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
Add Dockerfile #20
base: master
Are you sure you want to change the base?
Add Dockerfile #20
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
FROM ubuntu:20.04 | ||
ENV DEBIAN_FRONTEND=noninteractive | ||
WORKDIR /sqlite | ||
RUN apt-get update && apt-get install -y git | ||
RUN git clone https://github.com/sqlite/sqlite.git . | ||
RUN git checkout version-3.45.1 | ||
RUN apt-get install -y \ | ||
autoconf \ | ||
automake \ | ||
bison \ | ||
libtool \ | ||
make \ | ||
tcl \ | ||
pkg-config \ | ||
gcc \ | ||
g++ \ | ||
tcl-dev \ | ||
zlib1g-dev \ | ||
tcl8.6-dev \ | ||
tar | ||
RUN cd /sqlite \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to change directory into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the same reason as mentioned above, what we do is guide the LLM to generate, but it doesn't completely determine what it outputs. The purpose of our job is to help novice users get started with a project quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, so you're using AI to generate this |
||
&& git fetch \ | ||
&& git checkout version-3.45.1 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some reason you're checking out this tag twice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no other reason, simply because this Dockerfile is what we instructed the LLM to generate, and there may be a lack of checking for this step after the build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think there's missing context that you could document in the pull-request message. |
||
&& ./configure \ | ||
&& make \ | ||
&& make sqlite3.c \ | ||
&& make devtest | ||
EXPOSE 8080 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like a sane default, but should this port be configurable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This port is a configurable value. It can be specified by the user before the tool starts running. If the user does not specify it, the tool looks for a possible port in the project file, and if it does not find one, it uses the default port. After the Dockerfile is generated, you can also modify the port number according to your needs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue that the configurability of the port is worth documenting. |
||
CMD ["/bin/bash"] |
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 don't assume you intend this Dockerfile to be distributed as a standalone artifact. Assuming you're fine with this file being stored at the root of this repository, you could just copy the files you need (including git files) and copy those contents into
/sqlite
. Alternatively, you might assume the developer checks out the tag they want before building the docker image. As a new contributor, I'd be surprised that a particular version is enforced by the Dockerfile.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.
One of the features of our tool is that it allows the user to select a specific version when the tool starts running, so this version can be changed.
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.
Which tool?