-
Notifications
You must be signed in to change notification settings - Fork 446
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
feat!: assume BuildKit is available #913
Conversation
4f50e26
to
48ce135
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.
Just a very minor comment and we should be good to go. Thanks!
tutor/env.py
Outdated
@@ -54,7 +54,8 @@ def _prepare_environment() -> None: | |||
("HOST_USER_ID", utils.get_user_id()), | |||
("TUTOR_APP", __app__.replace("-", "_")), | |||
("TUTOR_VERSION", __version__), | |||
("is_buildkit_enabled", utils.is_buildkit_enabled), | |||
# For backwards compatability. Remove after Quince. |
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'm thinking we should add some sort of keyword in the comments to indicate future maintainers when to remove this. Something like DEPR v18
. I frequently struggle to re-discover whatever obsolete parts of the code need to be deleted from one major release to the next. What do you think?
That keyword should also be added to tutor.images.build (see change below).
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.
@regisb Good thinking. Though I think DEPR v18
would leave room for ambiguity--is it deprecated in v18, or should it be removed in v18, or removed after v18?
How about REMOVE-AFTER-Vxx
? For example, for this template filter, we're leaving it in for Quince (v16) but it'll be gone in Redwood, so it'd be # REMOVE-AFTER-V16
.
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.
"REMOVE-AFTER" looks great to me!
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.
Great, I just pushed a new commit: 551c631
BuildKit replaces and improves upon the legacy Docker builder, which was deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us to seriously simplify the Dockerfile, and it also unlocks future build performance improvements based on BuildKit-specific features such as: * COPY --link * Multiple build contexts * More aggressive parallelization during the build process The Docker versions which Tutor recommends (v20+) all come with BuildKit, so this should be a seamless transition for Tutor users who have been installing the recommended version. We leave the ``is_buildkit_enabled`` template variable in place in order to avoid breaking plugins, but we now just set it to ``True`` as a constant. It will be removed some time after Quince, so plugins should remove any uses of the variable from their Dockerfiles. Relevant discussion: overhangio#868 (comment)
48ce135
to
255c157
Compare
255c157
to
551c631
Compare
@regisb Did you want to re-review this, or is it good to go? |
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.
thanks Kyle!
Description
BuildKit replaces and improves upon the legacy Docker builder, which was deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us to seriously simplify the Dockerfile, and it also unlocks future build performance improvements based on BuildKit-specific features such as:
The Docker versions which Tutor recommends (v20+) all come with BuildKit, so this should be a seamless transition for Tutor users who have been installing the recommended version.
We leave the
is_buildkit_enabled
template variable in place in order to avoid breaking plugins, but we now just set it toTrue
as a constant. It will be removed some time after Quince, so plugins should remove any uses of the variable from their Dockerfiles.Relevant discussion:
#868 (comment)
Testing
I've been passively testing this for while. Specifically, I have been using kdmccormick#34 (which is based off of this branch) to work on Open edX features for the past couple months, using the following:
I haven't been testing the images in production-like tutor-local or tutor-k8s deployments. I don't believe the legacy->BuiltKit switch would have any impact that is specific to those modes, but I welcome additional testing if there is any concern.