-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove vitess/base
and vitess/k8s
Docker images
#15620
Conversation
Signed-off-by: Florent Poinsard <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
# Release a version. | ||
# This will generate a tar.gz file into the releases folder with the current source | ||
release: docker_base |
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 removed docker/release
and the corresponding make command as it seems to not be used and very ancient.
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.
Agreed. It was being used by the pre-GH release process.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15620 +/- ##
==========================================
+ Coverage 68.03% 68.12% +0.08%
==========================================
Files 1561 1556 -5
Lines 195526 194984 -542
==========================================
- Hits 133023 132827 -196
+ Misses 62503 62157 -346 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Florent Poinsard <[email protected]>
@@ -139,7 +139,7 @@ endif | |||
install: build | |||
# binaries | |||
mkdir -p "$${PREFIX}/bin" | |||
cp "$${VTROOTBIN}/"{mysqlctl,mysqlctld,vtorc,vtadmin,vtctld,vtctlclient,vtctldclient,vtgate,vttablet,vtbackup} "$${PREFIX}/bin/" | |||
cp "$${VTROOTBIN}/"{mysqlctl,mysqlctld,vtorc,vtadmin,vtctl,vtctld,vtctlclient,vtctldclient,vtgate,vttablet,vtbackup,vtexplain} "$${PREFIX}/bin/" |
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.
The image vitess/k8s
now relies on vitess/lite
, which runs make install
. In vitess/k8s
we copy these binaries so they are available for the binary-specific images. I had to add the missing binaries that we use in vitess/k8s
.
Signed-off-by: Florent Poinsard <[email protected]>
COPY --from=base /vt/bin/vtctl /vt/bin/ | ||
COPY --from=base /vt/bin/mysqlctl /vt/bin/ |
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.
Those two binaries are already present in vitess/lite
no need to copy them and no need for a base or bootstrap image.
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 good with this. I believe historically the k8s images were never migrated to use lite
instead of base
and this just fixes that. I can't think of a reason why someone would want to use base
any more.
Let's get @derekperkins's take on this before we merge.
Going through the checklist - does this affect vitess-operator in any way? |
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.
LGTM. We could go even further I think and eliminate the k8s
container, and build all of the subsequent containers from lite
directly. It originally existed mostly because lite
didn't have everything, and base
was too large to work with. Probably best to do that in a separate PR
If no one is using these images we may be able to skip the release notes too. |
I don't think it does, even for our tests. But if it does we will know soon enough. |
Signed-off-by: Florent Poinsard <[email protected]>
Achieved via 0fa9fdb. |
vitess/base
Docker imagevitess/base
and vitess/k8s
Docker images
Signed-off-by: Florent Poinsard <[email protected]>
It does not hurt to add a mention in the release notes 😃 Done via a7d69df |
Description
This PR removes the
vitess/base
andvitess/k8s
images. From now on and in most cases the "root" image isvitess/bootstrap
for everything. Binary-specific images will be built from thevitess/lite
image.Related Issue(s)
vitess/base
and build binary-specific images fromvitess/lite
#15606Checklist