Skip to content
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.

Sign up for GitHub

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

chore: cleaning up final references to slim from skaffold #9616

Merged
merged 1 commit into from
Dec 13, 2024
Merged

chore: cleaning up final references to slim from skaffold #9616

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hack/update-lts-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ For gcloud there is no api endpoint to check for the latest version so the outpu
the machine running this command has the latest gcloud version. Manual modification of the script is required in cases where \
the version found by the script is incorrect."

for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts" "Dockerfile.deps.slim"; do
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

Since Dockerfile.deps.slim is being removed, could you please add a comment explaining why this file is no longer needed? This will help future maintainers understand the reasoning behind this removal and prevent potential confusion or accidental reintroduction. For example, you could mention if the slim variant is deprecated or if it's being replaced by another approach.

Suggested change
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do # Removed Dockerfile.deps.slim as it's no longer needed (reason)

sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
Comment on lines 118 to 120
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

Now that Dockerfile.deps.slim is removed, it's a good opportunity to review if these sed commands are still optimal. Since the loop now only iterates over two files instead of three, it might be simpler and more readable to replace the loop with two separate sed commands, one for each Dockerfile. This avoids the loop overhead and makes the script slightly easier to understand at a glance.

Suggested change
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile}
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts

Expand Down
Loading