-
Notifications
You must be signed in to change notification settings - Fork 647
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
DevContainer! #11443
base: release-v0.16.x
Are you sure you want to change the base?
DevContainer! #11443
Conversation
Build Artifacts
|
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.
If we're focused on development tooling here, I'd rather we avoid intersecting docker/base.dockerfile
until we've had time to decide how we'll eventually establish a shippable docker image for Kolibri.
RUN npm install --global yarn | ||
|
||
# Change working directory for the commands that follow. | ||
WORKDIR /kolibri |
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.
Presumably, one could have built an image of Kolibri, containing its source, with this file before this change. After this change, which removes COPY . /kolibri
, the base image no longer contains the source, which could have unknown effects 'downstream' on things that may rely on this.
The dev
version already copies the source but I think it's worth considering whether a container that is used to develop Kolibri, like using the DevContainer tool, needs to intersect or build on top of the same image potentially used to deploy Kolibri (something which we want to eventually support-- having a clear docker image for Kolibri).
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.
Initially, my thought process was on those lines so I kept the devcontainer dockerfile completely isolated from deployable dockerfiles. But then eventually it turned out that keeping maintainability in mind utilising the base dockerfile would be better so went that route.
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.
cc @rtibbles
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.
keeping maintainability in mind utilising the base dockerfile
These changes are backwards incompatible though, so you haven't kept any maintainability.
The two images, a dev container image and a deployable image, have some commonalities such as installing python requirements, but overall have different use cases and responsibilities. For instance, a deployable Kolibri image doesn't need node.js (an existing dilemna) because node.js is only needed for building static JS/CSS resources.
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.
Yeah, so having a separate dockerfile for devcontainer is better, isn't it? 🤔
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 discussed this a while back, and I asked that you think about this holistically considering we have the desire to make running Kolibri in a docker container more well documented and to potentially have a well-defined shippable image. As I said, the two use cases have commonalities in how define and build these docker images but they have different responsibilities. With base.dockerfile
and dev.dockerfile
we already have some of that implemented and separated, but it isn't perfect and it isn't adapted for dev containers. Your changes here have integrated dev containers with what we already have, but have targeted much of that to base
and thus muddled base
vs dev
. TL;DR we already have a separate dockerfile for development but your changes here only affect base.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.
I totally agree with you on this that the current approach of this PR has put everything in just the base.dockerfile
.
I think we need to decide on how our shippable docker image for Kolibri will look like so that we can decide whether we should build our devcontainer image on top of that or not. I think I should talk to David to sort this out as soon as we can...?
# Install only the absolutely-required packages to run Kolibri -- Git, Git-LFS, ip, ps and Node (plus npm). | ||
RUN DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -y ca-certificates curl gnupg git git-lfs procps iproute2 \ | ||
&& mkdir -p /etc/apt/keyrings \ | ||
&& curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | gpg --dearmor -o /etc/apt/keyrings/nodesource.gpg \ |
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.
There isn't much benefit to setting up nodesource as a apt-repo inside the image. Also, there are better ways to handle this in a docker file, to better take advantage of caching.
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.
Nodesource suggested setting up apt-repo so that apt update
and apt install
can figure out nodesource repo.
https://github.com/nodesource/distributions?tab=readme-ov-file#installation-instructions
Regarding caching, I tried to keep the commands in one RUN command to make best use of caching. If there's more we can do to further improve on that then please let me know and I will do it.
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.
Right, they'd suggest an apt-repo, but this is a container! It will likely only ever be installed once so it'll never use it for checking for updates!
For example, if the node.js version changes, this installs ca-certificates curl gnupg git git-lfs procps iproute2
all over again. So no this isn't making the best use of caching.
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.
Right, they'd suggest an apt-repo, but this is a container! It will likely only ever be installed once so it'll never use it for checking for updates!
So without apt-repo, how should we install node?
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.
Your changes here modified how we were doing it before, which was not to use the apt-repo.
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.
Yeah right, I will fix that with our refactor of base & dev dockerfiles.
docs: setting up dev containers
@bjester until we are deciding on dev vs base dockerfile stuff, whenever you feel like, please give the documentation a read. Documentation is complete and ready for review. We just need to finalise on our dockerfile. |
Just a comment, no expectations here. I'm not familiar with VSCode DevContainers yet, but I'm all for containerizing development to ease developer onboarding, setup, and deployment. When/if Kolibri is containerized, it would be nice to add a devfile.yaml to the project https://devfile.io/ and maybe make the whole dev workspace portable, importable, and sharable via Eclipse Che Workspaces. https://eclipse.dev/che/ I don't know if there are any synergies between VsCode DevContainers and containers via devfile.yaml but it would be interesting to find out. |
Summary
This is an early draft for the team to look into. To run Kolibri in a DevContainer follow the steps below after getting the PR locally:-
Ctrl + Shift + P
then search and select "rebuild and reopen in container". Wait for 5 mins or so, then you'll be presented with a contarised developer enviornment.TODO --
References
Closes #11585.
Closes #11368.
Closes #11214.
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)